* [PATCH 0/3] a few debug_ll fixes and improvements
@ 2017-10-02 2:06 Nicolas Pitre
2017-10-02 2:06 ` [PATCH 1/3] ARM: debug.S: move hexbuf to a writable section Nicolas Pitre
` (2 more replies)
0 siblings, 3 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-02 2:06 UTC (permalink / raw)
To: linux-arm-kernel
This includes a couple fixes and improvements for using the early console
with semihosting on a v7m processor.
arch/arm/boot/compressed/debug.S | 4 ++++
arch/arm/kernel/debug.S | 22 +++++++++++++++++++---
arch/arm/kernel/early_printk.c | 16 ++++++++++------
3 files changed, 33 insertions(+), 9 deletions(-)
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 1/3] ARM: debug.S: move hexbuf to a writable section
2017-10-02 2:06 [PATCH 0/3] a few debug_ll fixes and improvements Nicolas Pitre
@ 2017-10-02 2:06 ` Nicolas Pitre
2017-10-02 2:06 ` [PATCH 2/3] ARM: semihosting: use proper instruction on v7m processors Nicolas Pitre
2017-10-02 2:06 ` [PATCH 3/3] ARM: early_printk: use printascii() rather than printch() Nicolas Pitre
2 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-02 2:06 UTC (permalink / raw)
To: linux-arm-kernel
This was located in .text which is meant to be read-only. And in the XIP
case this shortcut simply doesn't work and may trigger a Flash controller
mode switch and crash the kernel. Move it to the .bss area.
Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
arch/arm/kernel/debug.S | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index ea9646cc2a..423f443258 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -55,7 +55,9 @@ ENDPROC(printhex4)
ENTRY(printhex2)
mov r1, #2
-printhex: adr r2, hexbuf
+printhex: adr r2, hexbuf_rel
+ ldr r3, [r2]
+ add r2, r2, r3
add r3, r2, r1
mov r1, #0
strb r1, [r3]
@@ -71,7 +73,11 @@ printhex: adr r2, hexbuf
b printascii
ENDPROC(printhex2)
-hexbuf: .space 16
+ .pushsection .bss
+hexbuf_addr: .space 16
+ .popsection
+ .align
+hexbuf_rel: .long hexbuf_addr - .
.ltorg
@@ -120,7 +126,9 @@ ENTRY(printascii)
ENDPROC(printascii)
ENTRY(printch)
- adr r1, hexbuf
+ adr r1, hexbuf_rel
+ ldr r2, [r1]
+ add r1, r1, r2
strb r0, [r1]
mov r0, #0x03 @ SYS_WRITEC
ARM( svc #0x123456 )
--
2.9.5
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 2/3] ARM: semihosting: use proper instruction on v7m processors
2017-10-02 2:06 [PATCH 0/3] a few debug_ll fixes and improvements Nicolas Pitre
2017-10-02 2:06 ` [PATCH 1/3] ARM: debug.S: move hexbuf to a writable section Nicolas Pitre
@ 2017-10-02 2:06 ` Nicolas Pitre
2017-10-02 2:06 ` [PATCH 3/3] ARM: early_printk: use printascii() rather than printch() Nicolas Pitre
2 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-02 2:06 UTC (permalink / raw)
To: linux-arm-kernel
The svc instruction doesn't exist on v7m processors. Semihosting ops are
invoked with the bkpt instruction instead.
Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
arch/arm/boot/compressed/debug.S | 4 ++++
arch/arm/kernel/debug.S | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/arch/arm/boot/compressed/debug.S b/arch/arm/boot/compressed/debug.S
index 5392ee6333..8f6e37177d 100644
--- a/arch/arm/boot/compressed/debug.S
+++ b/arch/arm/boot/compressed/debug.S
@@ -23,7 +23,11 @@ ENTRY(putc)
strb r0, [r1]
mov r0, #0x03 @ SYS_WRITEC
ARM( svc #0x123456 )
+#ifdef CONFIG_CPU_V7M
+ THUMB( bkpt #0xab )
+#else
THUMB( svc #0xab )
+#endif
mov pc, lr
.align 2
1: .word _GLOBAL_OFFSET_TABLE_ - .
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index 423f443258..35145b618c 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -121,7 +121,11 @@ ENTRY(printascii)
mov r1, r0
mov r0, #0x04 @ SYS_WRITE0
ARM( svc #0x123456 )
+#ifdef CONFIG_CPU_V7M
+ THUMB( bkpt #0xab )
+#else
THUMB( svc #0xab )
+#endif
ret lr
ENDPROC(printascii)
@@ -132,7 +136,11 @@ ENTRY(printch)
strb r0, [r1]
mov r0, #0x03 @ SYS_WRITEC
ARM( svc #0x123456 )
+#ifdef CONFIG_CPU_V7M
+ THUMB( bkpt #0xab )
+#else
THUMB( svc #0xab )
+#endif
ret lr
ENDPROC(printch)
--
2.9.5
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-02 2:06 [PATCH 0/3] a few debug_ll fixes and improvements Nicolas Pitre
2017-10-02 2:06 ` [PATCH 1/3] ARM: debug.S: move hexbuf to a writable section Nicolas Pitre
2017-10-02 2:06 ` [PATCH 2/3] ARM: semihosting: use proper instruction on v7m processors Nicolas Pitre
@ 2017-10-02 2:06 ` Nicolas Pitre
2017-10-02 5:20 ` Uwe Kleine-König
2017-10-31 11:28 ` Chris Brandt
2 siblings, 2 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-02 2:06 UTC (permalink / raw)
To: linux-arm-kernel
With printch() the console messages are sent out one character at a time
which is agonizingly slow especially with semihosting as the whole trap
intercept, remote byte access, and system resume danse is performed for
every single character across a relatively slow remote debug connection.
Let's use printascii() to send a whole string at once. This is also going
to be more efficient, albeit to a quite lesser extent, with serial ports
as well.
Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
arch/arm/kernel/early_printk.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/kernel/early_printk.c b/arch/arm/kernel/early_printk.c
index 4307653696..9257736ec9 100644
--- a/arch/arm/kernel/early_printk.c
+++ b/arch/arm/kernel/early_printk.c
@@ -11,16 +11,20 @@
#include <linux/kernel.h>
#include <linux/console.h>
#include <linux/init.h>
+#include <linux/string.h>
-extern void printch(int);
+extern void printascii(const char *);
static void early_write(const char *s, unsigned n)
{
- while (n-- > 0) {
- if (*s == '\n')
- printch('\r');
- printch(*s);
- s++;
+ char buf[128];
+ while (n) {
+ unsigned l = min(n, sizeof(buf)-1);
+ memcpy(buf, s, l);
+ buf[l] = 0;
+ s += l;
+ n -= l;
+ printascii(buf);
}
}
--
2.9.5
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-02 2:06 ` [PATCH 3/3] ARM: early_printk: use printascii() rather than printch() Nicolas Pitre
@ 2017-10-02 5:20 ` Uwe Kleine-König
2017-10-02 14:09 ` Nicolas Pitre
2017-10-31 11:28 ` Chris Brandt
1 sibling, 1 reply; 45+ messages in thread
From: Uwe Kleine-König @ 2017-10-02 5:20 UTC (permalink / raw)
To: linux-arm-kernel
Hello Nicolas,
On Sun, Oct 01, 2017 at 10:06:18PM -0400, Nicolas Pitre wrote:
> With printch() the console messages are sent out one character at a time
> which is agonizingly slow especially with semihosting as the whole trap
> intercept, remote byte access, and system resume danse is performed for
s/danse/dance/ ?
> every single character across a relatively slow remote debug connection.
> Let's use printascii() to send a whole string at once. This is also going
> to be more efficient, albeit to a quite lesser extent, with serial ports
> as well.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
> arch/arm/kernel/early_printk.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/kernel/early_printk.c b/arch/arm/kernel/early_printk.c
> index 4307653696..9257736ec9 100644
> --- a/arch/arm/kernel/early_printk.c
> +++ b/arch/arm/kernel/early_printk.c
> @@ -11,16 +11,20 @@
> #include <linux/kernel.h>
> #include <linux/console.h>
> #include <linux/init.h>
> +#include <linux/string.h>
>
> -extern void printch(int);
> +extern void printascii(const char *);
>
> static void early_write(const char *s, unsigned n)
> {
> - while (n-- > 0) {
> - if (*s == '\n')
> - printch('\r');
> - printch(*s);
> - s++;
> + char buf[128];
> + while (n) {
> + unsigned l = min(n, sizeof(buf)-1);
> + memcpy(buf, s, l);
> + buf[l] = 0;
> + s += l;
> + n -= l;
> + printascii(buf);
I wonder if it is a usual case that n < 128 and s[n] == '\0'. If so this
could be tested for and then the memcpy could be dropped.
(The downside of course is that s[n] might also be something completely
different?)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-02 5:20 ` Uwe Kleine-König
@ 2017-10-02 14:09 ` Nicolas Pitre
0 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-02 14:09 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 2 Oct 2017, Uwe Kleine-K?nig wrote:
> Hello Nicolas,
>
> On Sun, Oct 01, 2017 at 10:06:18PM -0400, Nicolas Pitre wrote:
> > With printch() the console messages are sent out one character at a time
> > which is agonizingly slow especially with semihosting as the whole trap
> > intercept, remote byte access, and system resume danse is performed for
>
> s/danse/dance/ ?
>
> > every single character across a relatively slow remote debug connection.
> > Let's use printascii() to send a whole string at once. This is also going
> > to be more efficient, albeit to a quite lesser extent, with serial ports
> > as well.
> >
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> > arch/arm/kernel/early_printk.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/kernel/early_printk.c b/arch/arm/kernel/early_printk.c
> > index 4307653696..9257736ec9 100644
> > --- a/arch/arm/kernel/early_printk.c
> > +++ b/arch/arm/kernel/early_printk.c
> > @@ -11,16 +11,20 @@
> > #include <linux/kernel.h>
> > #include <linux/console.h>
> > #include <linux/init.h>
> > +#include <linux/string.h>
> >
> > -extern void printch(int);
> > +extern void printascii(const char *);
> >
> > static void early_write(const char *s, unsigned n)
> > {
> > - while (n-- > 0) {
> > - if (*s == '\n')
> > - printch('\r');
> > - printch(*s);
> > - s++;
> > + char buf[128];
> > + while (n) {
> > + unsigned l = min(n, sizeof(buf)-1);
> > + memcpy(buf, s, l);
> > + buf[l] = 0;
> > + s += l;
> > + n -= l;
> > + printascii(buf);
>
> I wonder if it is a usual case that n < 128 and s[n] == '\0'. If so this
> could be tested for and then the memcpy could be dropped.
I did try it. Unfortunately the passed string is not null terminated.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-02 2:06 ` [PATCH 3/3] ARM: early_printk: use printascii() rather than printch() Nicolas Pitre
2017-10-02 5:20 ` Uwe Kleine-König
@ 2017-10-31 11:28 ` Chris Brandt
2017-10-31 16:22 ` Nicolas Pitre
1 sibling, 1 reply; 45+ messages in thread
From: Chris Brandt @ 2017-10-31 11:28 UTC (permalink / raw)
To: linux-arm-kernel
On Sunday, October 01, 2017 1, Nicolas Pitre wrote:
> With printch() the console messages are sent out one character at a time
> which is agonizingly slow especially with semihosting as the whole trap
> intercept, remote byte access, and system resume danse is performed for
> every single character across a relatively slow remote debug connection.
> Let's use printascii() to send a whole string at once. This is also going
> to be more efficient, albeit to a quite lesser extent, with serial ports
> as well.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
> arch/arm/kernel/early_printk.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
Now that this patch has hit -next, I'm noticing an issue with it.
There are no carriage returns, just line feeds, which makes for a very
ugly display.
For example:
Booting Linux...
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.14.0-rc5-00014-g5689707d65e6 (chris at ubuntu) (gcc version 5.4.1 20161213 (Linaro GCC 5.4-2017.01)) #735 Mon Oct 30 12:59:37 EST 2017
[ 0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=58c53c7d
[ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[ 0.000000] OF: fdt: Machine model: RSKRZA1
[ 0.000000] debug: ignoring loglevel setting.
[ 0.000000] bootconsole [earlycon0] enabled
[ 0.000000] Memory policy: Data cache writeback
[ 0.000000] On node 0 totalpages: 8192
[ 0.000000] free_area_init_node: node 0, pgdat c003dfb4, node_mem_map c1fb8000
[ 0.000000] Normal zone: 64 pages used for memmap
[ 0.000000] Normal zone: 0 pages reserved
[ 0.000000] Normal zone: 8192 pages, LIFO batch:0
[ 0.000000] CPU: All CPU(s) started in SVC mode.
Of course as soon as the normal console driver is loaded later it boot,
everything looks fine again. But I use early_printk a lot, so I wonder
if I could get my carriage returns back.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 11:28 ` Chris Brandt
@ 2017-10-31 16:22 ` Nicolas Pitre
2017-10-31 16:38 ` Robin Murphy
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 16:22 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Chris Brandt wrote:
> On Sunday, October 01, 2017 1, Nicolas Pitre wrote:
> > With printch() the console messages are sent out one character at a time
> > which is agonizingly slow especially with semihosting as the whole trap
> > intercept, remote byte access, and system resume danse is performed for
> > every single character across a relatively slow remote debug connection.
> > Let's use printascii() to send a whole string at once. This is also going
> > to be more efficient, albeit to a quite lesser extent, with serial ports
> > as well.
> >
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> > arch/arm/kernel/early_printk.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
>
> Now that this patch has hit -next, I'm noticing an issue with it.
>
> There are no carriage returns, just line feeds, which makes for a very
> ugly display.
Hmmm....
If you look at printascii in arch/arm/kernel/debug.S you'll find the
following code:
1: waituart r2, r3
senduart r1, r3
busyuart r2, r3
teq r1, #'\n'
moveq r1, #'\r'
beq 1b
Why is that not working for you?
Are you using semihosting?
Do you have another printascii implementation?
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 16:22 ` Nicolas Pitre
@ 2017-10-31 16:38 ` Robin Murphy
2017-10-31 17:06 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Robin Murphy @ 2017-10-31 16:38 UTC (permalink / raw)
To: linux-arm-kernel
On 31/10/17 16:22, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Chris Brandt wrote:
>
>> On Sunday, October 01, 2017 1, Nicolas Pitre wrote:
>>> With printch() the console messages are sent out one character at a time
>>> which is agonizingly slow especially with semihosting as the whole trap
>>> intercept, remote byte access, and system resume danse is performed for
>>> every single character across a relatively slow remote debug connection.
>>> Let's use printascii() to send a whole string at once. This is also going
>>> to be more efficient, albeit to a quite lesser extent, with serial ports
>>> as well.
>>>
>>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>>> ---
>>> arch/arm/kernel/early_printk.c | 16 ++++++++++------
>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> Now that this patch has hit -next, I'm noticing an issue with it.
>>
>> There are no carriage returns, just line feeds, which makes for a very
>> ugly display.
>
> Hmmm....
>
> If you look at printascii in arch/arm/kernel/debug.S you'll find the
> following code:
>
> 1: waituart r2, r3
> senduart r1, r3
> busyuart r2, r3
> teq r1, #'\n'
> moveq r1, #'\r'
> beq 1b
>
> Why is that not working for you?
By inspection, the removed early_write() code inserted the '\r' before
the '\n' in the usual fashion; the printascii() code above ends up doing
the reverse, and I can imagine the atypical "\n\r" sequence probably
confuses some terminals trying to be clever with line ending detection.
Robin.
>
> Are you using semihosting?
>
> Do you have another printascii implementation?
>
>
> Nicolas
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 16:38 ` Robin Murphy
@ 2017-10-31 17:06 ` Nicolas Pitre
2017-10-31 17:16 ` Russell King - ARM Linux
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 17:06 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Robin Murphy wrote:
> On 31/10/17 16:22, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Chris Brandt wrote:
> >
> >> On Sunday, October 01, 2017 1, Nicolas Pitre wrote:
> >>> With printch() the console messages are sent out one character at a time
> >>> which is agonizingly slow especially with semihosting as the whole trap
> >>> intercept, remote byte access, and system resume danse is performed for
> >>> every single character across a relatively slow remote debug connection.
> >>> Let's use printascii() to send a whole string at once. This is also going
> >>> to be more efficient, albeit to a quite lesser extent, with serial ports
> >>> as well.
> >>>
> >>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> >>> ---
> >>> arch/arm/kernel/early_printk.c | 16 ++++++++++------
> >>> 1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> Now that this patch has hit -next, I'm noticing an issue with it.
> >>
> >> There are no carriage returns, just line feeds, which makes for a very
> >> ugly display.
> >
> > Hmmm....
> >
> > If you look at printascii in arch/arm/kernel/debug.S you'll find the
> > following code:
> >
> > 1: waituart r2, r3
> > senduart r1, r3
> > busyuart r2, r3
> > teq r1, #'\n'
> > moveq r1, #'\r'
> > beq 1b
> >
> > Why is that not working for you?
>
> By inspection, the removed early_write() code inserted the '\r' before
> the '\n' in the usual fashion; the printascii() code above ends up doing
> the reverse, and I can imagine the atypical "\n\r" sequence probably
> confuses some terminals trying to be clever with line ending detection.
That's easy to veryfy with this patch:
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index ea9646cc2a..40023a4871 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -79,18 +79,21 @@ hexbuf: .space 16
ENTRY(printascii)
addruart_current r3, r1, r2
- b 2f
-1: waituart r2, r3
- senduart r1, r3
- busyuart r2, r3
- teq r1, #'\n'
- moveq r1, #'\r'
- beq 1b
-2: teq r0, #0
+1: teq r0, #0
ldrneb r1, [r0], #1
teqne r1, #0
- bne 1b
- ret lr
+ reteq lr
+ teq r1, #'\n'
+ bne 2f
+ mov r1, '\r'
+ waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ mov r1, '\n'
+2: waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ b 1b
ENDPROC(printascii)
ENTRY(printch)
@Chris: does the above fix your display issue?
Nicolas
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 17:06 ` Nicolas Pitre
@ 2017-10-31 17:16 ` Russell King - ARM Linux
2017-10-31 17:48 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-10-31 17:16 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> That's easy to veryfy with this patch:
Unfortunately not that easy, this patch breaks printch.
> diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> index ea9646cc2a..40023a4871 100644
> --- a/arch/arm/kernel/debug.S
> +++ b/arch/arm/kernel/debug.S
> @@ -79,18 +79,21 @@ hexbuf: .space 16
>
The new code is:
> ENTRY(printascii)
> addruart_current r3, r1, r2
> +1: teq r0, #0
> ldrneb r1, [r0], #1
> teqne r1, #0
> + reteq lr
> + teq r1, #'\n'
> + bne 2f
> + mov r1, '\r'
> + waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + mov r1, '\n'
> +2: waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + b 1b
> ENDPROC(printascii)
and printch jumps to the 1: label with r0 = 0 and r1 = character. Your
change has the effect that the "reteq" will always be taken when called
by printch.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 17:16 ` Russell King - ARM Linux
@ 2017-10-31 17:48 ` Nicolas Pitre
2017-10-31 17:53 ` Russell King - ARM Linux
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 17:48 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> > That's easy to veryfy with this patch:
>
> Unfortunately not that easy, this patch breaks printch.
Right, missed that.
New patch below:
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index ea9646cc2a..0d1cef4b6e 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -79,25 +79,28 @@ hexbuf: .space 16
ENTRY(printascii)
addruart_current r3, r1, r2
- b 2f
-1: waituart r2, r3
- senduart r1, r3
- busyuart r2, r3
- teq r1, #'\n'
- moveq r1, #'\r'
- beq 1b
-2: teq r0, #0
+1: teq r0, #0
ldrneb r1, [r0], #1
teqne r1, #0
- bne 1b
- ret lr
+ reteq lr
+ teq r1, #'\n'
+ bne 2f
+ mov r1, '\r'
+ waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ mov r1, '\n'
+2: waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ b 1b
ENDPROC(printascii)
ENTRY(printch)
addruart_current r3, r1, r2
mov r1, r0
mov r0, #0
- b 1b
+ b 2b
ENDPROC(printch)
#ifdef CONFIG_MMU
>
> > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > index ea9646cc2a..40023a4871 100644
> > --- a/arch/arm/kernel/debug.S
> > +++ b/arch/arm/kernel/debug.S
> > @@ -79,18 +79,21 @@ hexbuf: .space 16
> >
>
> The new code is:
>
> > ENTRY(printascii)
> > addruart_current r3, r1, r2
> > +1: teq r0, #0
> > ldrneb r1, [r0], #1
> > teqne r1, #0
> > + reteq lr
> > + teq r1, #'\n'
> > + bne 2f
> > + mov r1, '\r'
> > + waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + mov r1, '\n'
> > +2: waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + b 1b
> > ENDPROC(printascii)
>
> and printch jumps to the 1: label with r0 = 0 and r1 = character. Your
> change has the effect that the "reteq" will always be taken when called
> by printch.
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up
>
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 17:48 ` Nicolas Pitre
@ 2017-10-31 17:53 ` Russell King - ARM Linux
2017-10-31 18:15 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-10-31 17:53 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 01:48:01PM -0400, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
>
> > On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> > > That's easy to veryfy with this patch:
> >
> > Unfortunately not that easy, this patch breaks printch.
>
> Right, missed that.
>
> New patch below:
>
> diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> index ea9646cc2a..0d1cef4b6e 100644
> --- a/arch/arm/kernel/debug.S
> +++ b/arch/arm/kernel/debug.S
> @@ -79,25 +79,28 @@ hexbuf: .space 16
>
> ENTRY(printascii)
> addruart_current r3, r1, r2
> - b 2f
> -1: waituart r2, r3
> - senduart r1, r3
> - busyuart r2, r3
> - teq r1, #'\n'
> - moveq r1, #'\r'
> - beq 1b
> -2: teq r0, #0
> +1: teq r0, #0
> ldrneb r1, [r0], #1
> teqne r1, #0
> - bne 1b
> - ret lr
> + reteq lr
> + teq r1, #'\n'
> + bne 2f
> + mov r1, '\r'
> + waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + mov r1, '\n'
> +2: waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + b 1b
> ENDPROC(printascii)
>
> ENTRY(printch)
> addruart_current r3, r1, r2
> mov r1, r0
> mov r0, #0
> - b 1b
> + b 2b
> ENDPROC(printch)
Still, not quite! printch('\n') with the old code would do this
(in terms of executed instructions):
mov r1, r0
mov r0, #0
b 1b
1: waituart r2, r3
senduart r1, r3
busyuart r2, r3
teq r1, #'\n'
moveq r1, #'\r'
beq 1b
1: waituart r2, r3
senduart r1, r3
busyuart r2, r3
teq r1, #'\n'
...
2: teq r0, #0
ret lr
So a printch('\n') produces "\n\r" on the UART. If we're fixing
printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
printch() should have the same fix, and should not truncate to
just '\n'.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 17:53 ` Russell King - ARM Linux
@ 2017-10-31 18:15 ` Nicolas Pitre
2017-10-31 18:20 ` Russell King - ARM Linux
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 18:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> On Tue, Oct 31, 2017 at 01:48:01PM -0400, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> >
> > > On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> > > > That's easy to veryfy with this patch:
> > >
> > > Unfortunately not that easy, this patch breaks printch.
> >
> > Right, missed that.
> >
> > New patch below:
> >
> > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > index ea9646cc2a..0d1cef4b6e 100644
> > --- a/arch/arm/kernel/debug.S
> > +++ b/arch/arm/kernel/debug.S
> > @@ -79,25 +79,28 @@ hexbuf: .space 16
> >
> > ENTRY(printascii)
> > addruart_current r3, r1, r2
> > - b 2f
> > -1: waituart r2, r3
> > - senduart r1, r3
> > - busyuart r2, r3
> > - teq r1, #'\n'
> > - moveq r1, #'\r'
> > - beq 1b
> > -2: teq r0, #0
> > +1: teq r0, #0
> > ldrneb r1, [r0], #1
> > teqne r1, #0
> > - bne 1b
> > - ret lr
> > + reteq lr
> > + teq r1, #'\n'
> > + bne 2f
> > + mov r1, '\r'
> > + waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + mov r1, '\n'
> > +2: waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + b 1b
> > ENDPROC(printascii)
> >
> > ENTRY(printch)
> > addruart_current r3, r1, r2
> > mov r1, r0
> > mov r0, #0
> > - b 1b
> > + b 2b
> > ENDPROC(printch)
>
> Still, not quite! printch('\n') with the old code would do this
> (in terms of executed instructions):
>
> mov r1, r0
> mov r0, #0
> b 1b
> 1: waituart r2, r3
> senduart r1, r3
> busyuart r2, r3
> teq r1, #'\n'
> moveq r1, #'\r'
> beq 1b
> 1: waituart r2, r3
> senduart r1, r3
> busyuart r2, r3
> teq r1, #'\n'
> ...
> 2: teq r0, #0
> ret lr
>
> So a printch('\n') produces "\n\r" on the UART. If we're fixing
> printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> printch() should have the same fix, and should not truncate to
> just '\n'.
OK... That's easy to achieve, but is it desirable?
That would imply that early_write() before my patch used to be wrong wrt
its princh() expectation as it did the extra \r manually on top. This
probably went unnoticed given that "\r\n\r" produces the same display
result as "\r\n".
IMHO the semantics of a single character should be just that: a single
character. And if some user relied on printch('\n') inserting the \r
automatically then this would have run into the same display issue we're
attempting to fix here. That might be why early_write() did the extra \r
itself.
Thing is: that only printch user user is now gone. So I'd fix the
semantic issue by simply removing printch altogether.
What do you think?
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 18:15 ` Nicolas Pitre
@ 2017-10-31 18:20 ` Russell King - ARM Linux
2017-10-31 18:35 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-10-31 18:20 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > On Tue, Oct 31, 2017 at 01:48:01PM -0400, Nicolas Pitre wrote:
> > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > >
> > > > On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> > > > > That's easy to veryfy with this patch:
> > > >
> > > > Unfortunately not that easy, this patch breaks printch.
> > >
> > > Right, missed that.
> > >
> > > New patch below:
> > >
> > > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > > index ea9646cc2a..0d1cef4b6e 100644
> > > --- a/arch/arm/kernel/debug.S
> > > +++ b/arch/arm/kernel/debug.S
> > > @@ -79,25 +79,28 @@ hexbuf: .space 16
> > >
> > > ENTRY(printascii)
> > > addruart_current r3, r1, r2
> > > - b 2f
> > > -1: waituart r2, r3
> > > - senduart r1, r3
> > > - busyuart r2, r3
> > > - teq r1, #'\n'
> > > - moveq r1, #'\r'
> > > - beq 1b
> > > -2: teq r0, #0
> > > +1: teq r0, #0
> > > ldrneb r1, [r0], #1
> > > teqne r1, #0
> > > - bne 1b
> > > - ret lr
> > > + reteq lr
> > > + teq r1, #'\n'
> > > + bne 2f
> > > + mov r1, '\r'
> > > + waituart r2, r3
> > > + senduart r1, r3
> > > + busyuart r2, r3
> > > + mov r1, '\n'
> > > +2: waituart r2, r3
> > > + senduart r1, r3
> > > + busyuart r2, r3
> > > + b 1b
> > > ENDPROC(printascii)
> > >
> > > ENTRY(printch)
> > > addruart_current r3, r1, r2
> > > mov r1, r0
> > > mov r0, #0
> > > - b 1b
> > > + b 2b
> > > ENDPROC(printch)
> >
> > Still, not quite! printch('\n') with the old code would do this
> > (in terms of executed instructions):
> >
> > mov r1, r0
> > mov r0, #0
> > b 1b
> > 1: waituart r2, r3
> > senduart r1, r3
> > busyuart r2, r3
> > teq r1, #'\n'
> > moveq r1, #'\r'
> > beq 1b
> > 1: waituart r2, r3
> > senduart r1, r3
> > busyuart r2, r3
> > teq r1, #'\n'
> > ...
> > 2: teq r0, #0
> > ret lr
> >
> > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > printch() should have the same fix, and should not truncate to
> > just '\n'.
>
> OK... That's easy to achieve, but is it desirable?
Yes - remember, these are supposed to be usable from assembly,
and we really don't want to have the complexity of:
mov r0, #'\r'
bl printch
mov r0, #'\n'
bl printch
each time we want to begin a new line.
> IMHO the semantics of a single character should be just that: a single
> character. And if some user relied on printch('\n') inserting the \r
> automatically then this would have run into the same display issue we're
> attempting to fix here. That might be why early_write() did the extra \r
> itself.
You're thinking about these functions as C functions that you'd call
from C code. That's not their primary purpose or intention, they're
for debugging assembly. They just happen to be re-used for the
early printk crap because people find these a convenient implementation.
> Thing is: that only printch user user is now gone. So I'd fix the
> semantic issue by simply removing printch altogether.
No. You could make the same argument for the printhex*() functions
too, but removing them means next time we need to stuff in some
debug that prints hex numbers before the MMU is up, we need to
reinvent those wheels yet again.
The point of this assembly is to provide a set of debugging functions
that can be used from assembly. They were never really meant for C
code to use.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 18:20 ` Russell King - ARM Linux
@ 2017-10-31 18:35 ` Nicolas Pitre
2017-10-31 19:12 ` Chris Brandt
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 18:35 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > > printch() should have the same fix, and should not truncate to
> > > just '\n'.
> >
> > OK... That's easy to achieve, but is it desirable?
>
> Yes - remember, these are supposed to be usable from assembly,
> and we really don't want to have the complexity of:
>
> mov r0, #'\r'
> bl printch
> mov r0, #'\n'
> bl printch
>
> each time we want to begin a new line.
Fine with me.
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index ea9646cc2a..01d746efff 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -79,25 +79,28 @@ hexbuf: .space 16
ENTRY(printascii)
addruart_current r3, r1, r2
- b 2f
-1: waituart r2, r3
- senduart r1, r3
- busyuart r2, r3
- teq r1, #'\n'
- moveq r1, #'\r'
- beq 1b
-2: teq r0, #0
+1: teq r0, #0
ldrneb r1, [r0], #1
teqne r1, #0
- bne 1b
- ret lr
+ reteq lr
+2: teq r1, #'\n'
+ bne 3f
+ mov r1, '\r'
+ waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ mov r1, '\n'
+3: waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ b 1b
ENDPROC(printascii)
ENTRY(printch)
addruart_current r3, r1, r2
mov r1, r0
mov r0, #0
- b 1b
+ b 2b
ENDPROC(printch)
#ifdef CONFIG_MMU
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 18:35 ` Nicolas Pitre
@ 2017-10-31 19:12 ` Chris Brandt
2017-10-31 19:28 ` Nicolas Pitre
2017-11-02 0:09 ` Russell King - ARM Linux
0 siblings, 2 replies; 45+ messages in thread
From: Chris Brandt @ 2017-10-31 19:12 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday, October 31, 2017, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > > > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > > > printch() should have the same fix, and should not truncate to
> > > > just '\n'.
> > >
> > > OK... That's easy to achieve, but is it desirable?
> >
> > Yes - remember, these are supposed to be usable from assembly,
> > and we really don't want to have the complexity of:
> >
> > mov r0, #'\r'
> > bl printch
> > mov r0, #'\n'
> > bl printch
> >
> > each time we want to begin a new line.
>
> Fine with me.
>
> diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> index ea9646cc2a..01d746efff 100644
> --- a/arch/arm/kernel/debug.S
> +++ b/arch/arm/kernel/debug.S
> @@ -79,25 +79,28 @@ hexbuf: .space 16
>
> ENTRY(printascii)
> addruart_current r3, r1, r2
> - b 2f
> -1: waituart r2, r3
> - senduart r1, r3
> - busyuart r2, r3
> - teq r1, #'\n'
> - moveq r1, #'\r'
> - beq 1b
> -2: teq r0, #0
> +1: teq r0, #0
> ldrneb r1, [r0], #1
> teqne r1, #0
> - bne 1b
> - ret lr
> + reteq lr
> +2: teq r1, #'\n'
> + bne 3f
> + mov r1, '\r'
> + waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + mov r1, '\n'
> +3: waituart r2, r3
> + senduart r1, r3
> + busyuart r2, r3
> + b 1b
> ENDPROC(printascii)
>
> ENTRY(printch)
> addruart_current r3, r1, r2
> mov r1, r0
> mov r0, #0
> - b 1b
> + b 2b
> ENDPROC(printch)
>
> #ifdef CONFIG_MMU
This patch worked for me.
I get my carriage returns again.
Thanks.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 19:12 ` Chris Brandt
@ 2017-10-31 19:28 ` Nicolas Pitre
2017-10-31 21:50 ` Russell King - ARM Linux
2017-11-02 0:09 ` Russell King - ARM Linux
1 sibling, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 19:28 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Chris Brandt wrote:
> On Tuesday, October 31, 2017, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> > > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > > > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > > > > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > > > > printch() should have the same fix, and should not truncate to
> > > > > just '\n'.
> > > >
> > > > OK... That's easy to achieve, but is it desirable?
> > >
> > > Yes - remember, these are supposed to be usable from assembly,
> > > and we really don't want to have the complexity of:
> > >
> > > mov r0, #'\r'
> > > bl printch
> > > mov r0, #'\n'
> > > bl printch
> > >
> > > each time we want to begin a new line.
> >
> > Fine with me.
> >
> > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > index ea9646cc2a..01d746efff 100644
> > --- a/arch/arm/kernel/debug.S
> > +++ b/arch/arm/kernel/debug.S
> > @@ -79,25 +79,28 @@ hexbuf: .space 16
> >
> > ENTRY(printascii)
> > addruart_current r3, r1, r2
> > - b 2f
> > -1: waituart r2, r3
> > - senduart r1, r3
> > - busyuart r2, r3
> > - teq r1, #'\n'
> > - moveq r1, #'\r'
> > - beq 1b
> > -2: teq r0, #0
> > +1: teq r0, #0
> > ldrneb r1, [r0], #1
> > teqne r1, #0
> > - bne 1b
> > - ret lr
> > + reteq lr
> > +2: teq r1, #'\n'
> > + bne 3f
> > + mov r1, '\r'
> > + waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + mov r1, '\n'
> > +3: waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + b 1b
> > ENDPROC(printascii)
> >
> > ENTRY(printch)
> > addruart_current r3, r1, r2
> > mov r1, r0
> > mov r0, #0
> > - b 1b
> > + b 2b
> > ENDPROC(printch)
> >
> > #ifdef CONFIG_MMU
>
>
> This patch worked for me.
> I get my carriage returns again.
Good! Queued as patch #8717.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 19:28 ` Nicolas Pitre
@ 2017-10-31 21:50 ` Russell King - ARM Linux
2017-10-31 23:35 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-10-31 21:50 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 03:28:11PM -0400, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Chris Brandt wrote:
> > This patch worked for me.
> > I get my carriage returns again.
>
> Good! Queued as patch #8717.
I was going to say that there's another implementation of the
same in arch/arm/boot/compressed/head.S, and if we fix one we
should apply the same fix to the other.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 21:50 ` Russell King - ARM Linux
@ 2017-10-31 23:35 ` Nicolas Pitre
2017-10-31 23:50 ` Russell King - ARM Linux
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-10-31 23:35 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> On Tue, Oct 31, 2017 at 03:28:11PM -0400, Nicolas Pitre wrote:
> > Good! Queued as patch #8717.
>
> I was going to say that there's another implementation of the
> same in arch/arm/boot/compressed/head.S, and if we fix one we
> should apply the same fix to the other.
Let's make a separate patch for that one. Something like:
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 8a756870c2..28a48ad3dd 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -1296,19 +1296,24 @@ phex: adr r3, phexbuf
@ puts corrupts {r0, r1, r2, r3}
puts: loadsp r3, r1
-1: ldrb r2, [r0], #1
- teq r2, #0
+1: teq r0, #0
+ ldrneb r2, [r0], #1
+ teqne r2, #0
moveq pc, lr
-2: writeb r2, r3
+2: teq r2, #'\n'
+ bne 4f
+ mov r2, #'\r'
+ writeb r2, r3
mov r1, #0x00020000
3: subs r1, r1, #1
bne 3b
- teq r2, #'\n'
- moveq r2, #'\r'
- beq 2b
- teq r0, #0
- bne 1b
- mov pc, lr
+ mov r2, #'\n'
+4: writeb r2, r3
+ mov r1, #0x00020000
+5: subs r1, r1, #1
+ bne 5b
+ b 1b
+
@ putc corrupts {r0, r1, r2, r3}
putc:
mov r2, r0
Please review as this is untested.
Nicolas
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 23:35 ` Nicolas Pitre
@ 2017-10-31 23:50 ` Russell King - ARM Linux
0 siblings, 0 replies; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-10-31 23:50 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 07:35:56PM -0400, Nicolas Pitre wrote:
> On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
>
> > On Tue, Oct 31, 2017 at 03:28:11PM -0400, Nicolas Pitre wrote:
> > > Good! Queued as patch #8717.
> >
> > I was going to say that there's another implementation of the
> > same in arch/arm/boot/compressed/head.S, and if we fix one we
> > should apply the same fix to the other.
>
> Let's make a separate patch for that one. Something like:
>
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 8a756870c2..28a48ad3dd 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -1296,19 +1296,24 @@ phex: adr r3, phexbuf
>
> @ puts corrupts {r0, r1, r2, r3}
> puts: loadsp r3, r1
> -1: ldrb r2, [r0], #1
> - teq r2, #0
> +1: teq r0, #0
> + ldrneb r2, [r0], #1
> + teqne r2, #0
> moveq pc, lr
> -2: writeb r2, r3
> +2: teq r2, #'\n'
> + bne 4f
> + mov r2, #'\r'
> + writeb r2, r3
> mov r1, #0x00020000
> 3: subs r1, r1, #1
> bne 3b
> - teq r2, #'\n'
> - moveq r2, #'\r'
> - beq 2b
> - teq r0, #0
> - bne 1b
> - mov pc, lr
> + mov r2, #'\n'
> +4: writeb r2, r3
> + mov r1, #0x00020000
> +5: subs r1, r1, #1
> + bne 5b
> + b 1b
> +
> @ putc corrupts {r0, r1, r2, r3}
> putc:
> mov r2, r0
>
> Please review as this is untested.
Looks good from a quick read through, thanks.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-10-31 19:12 ` Chris Brandt
2017-10-31 19:28 ` Nicolas Pitre
@ 2017-11-02 0:09 ` Russell King - ARM Linux
2017-11-02 3:59 ` Nicolas Pitre
1 sibling, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 0:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 31, 2017 at 07:12:32PM +0000, Chris Brandt wrote:
> On Tuesday, October 31, 2017, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> > > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > > > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > > > > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > > > > printch() should have the same fix, and should not truncate to
> > > > > just '\n'.
> > > >
> > > > OK... That's easy to achieve, but is it desirable?
> > >
> > > Yes - remember, these are supposed to be usable from assembly,
> > > and we really don't want to have the complexity of:
> > >
> > > mov r0, #'\r'
> > > bl printch
> > > mov r0, #'\n'
> > > bl printch
> > >
> > > each time we want to begin a new line.
> >
> > Fine with me.
> >
> > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > index ea9646cc2a..01d746efff 100644
> > --- a/arch/arm/kernel/debug.S
> > +++ b/arch/arm/kernel/debug.S
> > @@ -79,25 +79,28 @@ hexbuf: .space 16
> >
> > ENTRY(printascii)
> > addruart_current r3, r1, r2
> > - b 2f
> > -1: waituart r2, r3
> > - senduart r1, r3
> > - busyuart r2, r3
> > - teq r1, #'\n'
> > - moveq r1, #'\r'
> > - beq 1b
> > -2: teq r0, #0
> > +1: teq r0, #0
> > ldrneb r1, [r0], #1
> > teqne r1, #0
> > - bne 1b
> > - ret lr
> > + reteq lr
> > +2: teq r1, #'\n'
> > + bne 3f
> > + mov r1, '\r'
> > + waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + mov r1, '\n'
> > +3: waituart r2, r3
> > + senduart r1, r3
> > + busyuart r2, r3
> > + b 1b
> > ENDPROC(printascii)
> >
> > ENTRY(printch)
> > addruart_current r3, r1, r2
> > mov r1, r0
> > mov r0, #0
> > - b 1b
> > + b 2b
> > ENDPROC(printch)
> >
> > #ifdef CONFIG_MMU
>
>
> This patch worked for me.
> I get my carriage returns again.
Sorry, but no. This is crap.
The kernelci.org test resulting from the tree I pushed out this evening
with both of the patches in is very unhappy:
42 arch/arm/kernel/debug.S:98: Error: immediate expression requires a # prefix -- `mov r1,10'
42 arch/arm/kernel/debug.S:94: Error: immediate expression requires a # prefix -- `mov r1,13'
I can't believe that anyone actually build-tested this patch as it
stands - maybe, Chris, you just think you did but you ended up
testing something else? Or maybe your binutils is broken because
it now accepts constants without the preceding '#' ?
Shrug, whatever, Nico's patches are broken, and it's way too late to
go through another round of potential broken-ness. Dropping both
from my tree until after the merge window. Sorry.
Last minute fixes hardly ever work out.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 0:09 ` Russell King - ARM Linux
@ 2017-11-02 3:59 ` Nicolas Pitre
2017-11-02 4:16 ` Nicolas Pitre
2017-11-02 11:06 ` Chris Brandt
0 siblings, 2 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 3:59 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Russell King - ARM Linux wrote:
> On Tue, Oct 31, 2017 at 07:12:32PM +0000, Chris Brandt wrote:
> > On Tuesday, October 31, 2017, Nicolas Pitre wrote:
> > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > > On Tue, Oct 31, 2017 at 02:15:14PM -0400, Nicolas Pitre wrote:
> > > > > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > > > > > So a printch('\n') produces "\n\r" on the UART. If we're fixing
> > > > > > printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> > > > > > printch() should have the same fix, and should not truncate to
> > > > > > just '\n'.
> > > > >
> > > > > OK... That's easy to achieve, but is it desirable?
> > > >
> > > > Yes - remember, these are supposed to be usable from assembly,
> > > > and we really don't want to have the complexity of:
> > > >
> > > > mov r0, #'\r'
> > > > bl printch
> > > > mov r0, #'\n'
> > > > bl printch
> > > >
> > > > each time we want to begin a new line.
> > >
> > > Fine with me.
> > >
> > > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > > index ea9646cc2a..01d746efff 100644
> > > --- a/arch/arm/kernel/debug.S
> > > +++ b/arch/arm/kernel/debug.S
> > > @@ -79,25 +79,28 @@ hexbuf: .space 16
> > >
> > > ENTRY(printascii)
> > > addruart_current r3, r1, r2
> > > - b 2f
> > > -1: waituart r2, r3
> > > - senduart r1, r3
> > > - busyuart r2, r3
> > > - teq r1, #'\n'
> > > - moveq r1, #'\r'
> > > - beq 1b
> > > -2: teq r0, #0
> > > +1: teq r0, #0
> > > ldrneb r1, [r0], #1
> > > teqne r1, #0
> > > - bne 1b
> > > - ret lr
> > > + reteq lr
> > > +2: teq r1, #'\n'
> > > + bne 3f
> > > + mov r1, '\r'
> > > + waituart r2, r3
> > > + senduart r1, r3
> > > + busyuart r2, r3
> > > + mov r1, '\n'
> > > +3: waituart r2, r3
> > > + senduart r1, r3
> > > + busyuart r2, r3
> > > + b 1b
> > > ENDPROC(printascii)
> > >
> > > ENTRY(printch)
> > > addruart_current r3, r1, r2
> > > mov r1, r0
> > > mov r0, #0
> > > - b 1b
> > > + b 2b
> > > ENDPROC(printch)
> > >
> > > #ifdef CONFIG_MMU
> >
> >
> > This patch worked for me.
> > I get my carriage returns again.
>
> Sorry, but no. This is crap.
>
> The kernelci.org test resulting from the tree I pushed out this evening
> with both of the patches in is very unhappy:
>
> 42 arch/arm/kernel/debug.S:98: Error: immediate expression requires a # prefix -- `mov r1,10'
> 42 arch/arm/kernel/debug.S:94: Error: immediate expression requires a # prefix -- `mov r1,13'
>
> I can't believe that anyone actually build-tested this patch as it
> stands - maybe, Chris, you just think you did but you ended up
> testing something else? Or maybe your binutils is broken because
> it now accepts constants without the preceding '#' ?
Well... I don't know what happened with Chris' testing either.
I *thought* I build tested it, but my .config had
CONFIG_DEBUG_SEMIHOSTING=y.
Oh well...
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 3:59 ` Nicolas Pitre
@ 2017-11-02 4:16 ` Nicolas Pitre
2017-11-02 11:09 ` Russell King - ARM Linux
2017-11-02 11:06 ` Chris Brandt
1 sibling, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 4:16 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 1 Nov 2017, Nicolas Pitre wrote:
> Well... I don't know what happened with Chris' testing either.
>
> I *thought* I build tested it, but my .config had
> CONFIG_DEBUG_SEMIHOSTING=y.
Here's the fixed patch. I won't submit it to the patch system before we
understand how the previous one worked for Chris.
----- >8
Subject: debug printch/printascii: translate '\n' to "\r\n" not "\n\r"
Some terminals apparently have issues with "\n\r" and mess up the
display. Let's use the traditional "\r\n" ordering.
Signed-off-by: Nicolas Pitre <nico@linaro.org>
Reported-by: Chris Brandt <Chris.Brandt@renesas.com>
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index ea9646cc2a..01d746efff 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -79,25 +79,28 @@ hexbuf: .space 16
ENTRY(printascii)
addruart_current r3, r1, r2
- b 2f
-1: waituart r2, r3
- senduart r1, r3
- busyuart r2, r3
- teq r1, #'\n'
- moveq r1, #'\r'
- beq 1b
-2: teq r0, #0
+1: teq r0, #0
ldrneb r1, [r0], #1
teqne r1, #0
- bne 1b
- ret lr
+ reteq lr
+2: teq r1, #'\n'
+ bne 3f
+ mov r1, #'\r'
+ waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ mov r1, #'\n'
+3: waituart r2, r3
+ senduart r1, r3
+ busyuart r2, r3
+ b 1b
ENDPROC(printascii)
ENTRY(printch)
addruart_current r3, r1, r2
mov r1, r0
mov r0, #0
- b 1b
+ b 2b
ENDPROC(printch)
#ifdef CONFIG_MMU
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 3:59 ` Nicolas Pitre
2017-11-02 4:16 ` Nicolas Pitre
@ 2017-11-02 11:06 ` Chris Brandt
2017-11-02 11:20 ` Russell King - ARM Linux
1 sibling, 1 reply; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 11:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, November 01, 2017 1, Nicolas Pitre wrote:
> > > This patch worked for me.
> > > I get my carriage returns again.
> >
> > Sorry, but no. This is crap.
> >
> > The kernelci.org test resulting from the tree I pushed out this evening
> > with both of the patches in is very unhappy:
> >
> > 42 arch/arm/kernel/debug.S:98: Error: immediate expression
> requires a # prefix -- `mov r1,10'
> > 42 arch/arm/kernel/debug.S:94: Error: immediate expression
> requires a # prefix -- `mov r1,13'
> >
> > I can't believe that anyone actually build-tested this patch as it
> > stands - maybe, Chris, you just think you did but you ended up
> > testing something else? Or maybe your binutils is broken because
> > it now accepts constants without the preceding '#' ?
>
> Well... I don't know what happened with Chris' testing either.
So, just to show I'm not crazy...
# Yes, patch was applied:
$ grep "mov r1" arch/arm/kernel/debug.S
mov r1, #8
mov r1, #4
mov r1, #2
mov r1, #0
mov r1, '\r' <<<<<<<<<<
mov r1, '\n' <<<<<<<<<<
mov r1, r0
mov r1, r0
# Yes it builds:
$ touch arch/arm/kernel/debug.S
$ make -j8 O=.out uImage LOADADDR=0x08008000
make[1]: Entering directory '/home/renesas/tools/upstream/renesas-drivers/.out'
CHK include/config/kernel.release
GEN ./Makefile
CHK include/generated/uapi/linux/version.h
CHK scripts/mod/devicetable-offsets.h
UPD include/config/kernel.release
Using .. as source for kernel
CHK include/generated/utsrelease.h
UPD include/generated/utsrelease.h
CHK include/generated/timeconst.h
CHK include/generated/bounds.h
CHK include/generated/asm-offsets.h
CALL ../scripts/checksyscalls.sh
CHK include/generated/compile.h
CC init/version.o
AS arch/arm/kernel/debug.o <<<<<<<<<<
# Have a look@the output:
$ arm-linux-gnueabihf-objdump -SdCg .out/arch/arm/kernel/debug.o > /tmp/debug.lst
$ cat /tmp/debug.lst
------------------------------------------------------------
mov r1, '\r' <<<<<<<<<<
70: f04f 010d mov.w r1, #13 <<<<<<<<<<
waituart r2, r3
74: 8a1a ldrh r2, [r3, #16]
76: f012 0f20 tst.w r2, #32
7a: d0fb beq.n 74 <printascii+0x2c>
senduart r1, r3
7c: 7319 strb r1, [r3, #12]
7e: 8a19 ldrh r1, [r3, #16]
80: f021 0140 bic.w r1, r1, #64 ; 0x40
84: 8219 strh r1, [r3, #16]
busyuart r2, r3
86: 8a1a ldrh r2, [r3, #16]
88: f012 0f40 tst.w r2, #64 ; 0x40
8c: d0fb beq.n 86 <printascii+0x3e>
mov r1, '\n' <<<<<<<<<<
8e: f04f 010a mov.w r1, #10 <<<<<<<<<<
------------------------------------------------------------
So, the compiler realized what you wanted to do even if there wasn't a
# in front of the constant.
$ arm-linux-gnueabihf-gcc --version
arm-linux-gnueabihf-gcc (Linaro GCC 5.4-2017.01) 5.4.1 20161213
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 4:16 ` Nicolas Pitre
@ 2017-11-02 11:09 ` Russell King - ARM Linux
2017-11-02 15:12 ` Nicolas Pitre
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 11:09 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 12:16:54AM -0400, Nicolas Pitre wrote:
> On Wed, 1 Nov 2017, Nicolas Pitre wrote:
>
> > Well... I don't know what happened with Chris' testing either.
> >
> > I *thought* I build tested it, but my .config had
> > CONFIG_DEBUG_SEMIHOSTING=y.
>
> Here's the fixed patch. I won't submit it to the patch system before we
> understand how the previous one worked for Chris.
As we have seen, there's risk involved with merging apparently tested
fixes, and it can result in major build breakage.
Today is the last linux-next fetch before the potential opening of the
merge window on Sunday. I'd prefer not to merge anything in the next
four days, but instead let things settle ready for that.
Most of last night's builds for my autobuilder failed due to this
problem (I didn't have a chance to regenerate that tree after kernelci
reported the problem), so I'm going to have to wait until tonight's
builds to see how things are - and that's starting to leave precious
little available time before the merge window. I don't run the
builder during the day as it eats around six hours of time.
So, in short, my tree is now closed in expectation of 4.14 on Sunday.
However, if you want me to remove "8705/1: early_printk: use printascii()
rather than printch()" I can do that, but only if you tell me before
4pm UTC today.
If we get 4.14-rc8 instead, I'll re-open for a few days for this.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 11:06 ` Chris Brandt
@ 2017-11-02 11:20 ` Russell King - ARM Linux
2017-11-02 11:28 ` Chris Brandt
0 siblings, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 11:20 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 11:06:54AM +0000, Chris Brandt wrote:
> On Wednesday, November 01, 2017 1, Nicolas Pitre wrote:
> > > > This patch worked for me.
> > > > I get my carriage returns again.
> > >
> > > Sorry, but no. This is crap.
> > >
> > > The kernelci.org test resulting from the tree I pushed out this evening
> > > with both of the patches in is very unhappy:
> > >
> > > 42 arch/arm/kernel/debug.S:98: Error: immediate expression
> > requires a # prefix -- `mov r1,10'
> > > 42 arch/arm/kernel/debug.S:94: Error: immediate expression
> > requires a # prefix -- `mov r1,13'
> > >
> > > I can't believe that anyone actually build-tested this patch as it
> > > stands - maybe, Chris, you just think you did but you ended up
> > > testing something else? Or maybe your binutils is broken because
> > > it now accepts constants without the preceding '#' ?
> >
> > Well... I don't know what happened with Chris' testing either.
>
>
> So, just to show I'm not crazy...
>
>
> # Yes, patch was applied:
> $ grep "mov r1" arch/arm/kernel/debug.S
> mov r1, #8
> mov r1, #4
> mov r1, #2
> mov r1, #0
> mov r1, '\r' <<<<<<<<<<
> mov r1, '\n' <<<<<<<<<<
> mov r1, r0
> mov r1, r0
>
> # Yes it builds:
> $ touch arch/arm/kernel/debug.S
> $ make -j8 O=.out uImage LOADADDR=0x08008000
> make[1]: Entering directory '/home/renesas/tools/upstream/renesas-drivers/.out'
> CHK include/config/kernel.release
> GEN ./Makefile
> CHK include/generated/uapi/linux/version.h
> CHK scripts/mod/devicetable-offsets.h
> UPD include/config/kernel.release
> Using .. as source for kernel
> CHK include/generated/utsrelease.h
> UPD include/generated/utsrelease.h
> CHK include/generated/timeconst.h
> CHK include/generated/bounds.h
> CHK include/generated/asm-offsets.h
> CALL ../scripts/checksyscalls.sh
> CHK include/generated/compile.h
> CC init/version.o
> AS arch/arm/kernel/debug.o <<<<<<<<<<
>
>
> # Have a look at the output:
> $ arm-linux-gnueabihf-objdump -SdCg .out/arch/arm/kernel/debug.o > /tmp/debug.lst
> $ cat /tmp/debug.lst
>
> ------------------------------------------------------------
> mov r1, '\r' <<<<<<<<<<
> 70: f04f 010d mov.w r1, #13 <<<<<<<<<<
> waituart r2, r3
> 74: 8a1a ldrh r2, [r3, #16]
> 76: f012 0f20 tst.w r2, #32
> 7a: d0fb beq.n 74 <printascii+0x2c>
> senduart r1, r3
> 7c: 7319 strb r1, [r3, #12]
> 7e: 8a19 ldrh r1, [r3, #16]
> 80: f021 0140 bic.w r1, r1, #64 ; 0x40
> 84: 8219 strh r1, [r3, #16]
> busyuart r2, r3
> 86: 8a1a ldrh r2, [r3, #16]
> 88: f012 0f40 tst.w r2, #64 ; 0x40
> 8c: d0fb beq.n 86 <printascii+0x3e>
> mov r1, '\n' <<<<<<<<<<
> 8e: f04f 010a mov.w r1, #10 <<<<<<<<<<
> ------------------------------------------------------------
>
>
> So, the compiler realized what you wanted to do even if there wasn't a
> # in front of the constant.
>
>
> $ arm-linux-gnueabihf-gcc --version
> arm-linux-gnueabihf-gcc (Linaro GCC 5.4-2017.01) 5.4.1 20161213
> Copyright (C) 2015 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
The compiler is only involved for the C pre-processor front-end. It's
not involved in parsing the resulting assembly - as far as gcc is
concerned, it could be forth in the post-processed file.
GCC will then pass the post-processed output to binutils 'as' to do the
actual assembly, and that's what should complain.
Most people's assemblers will object to the second instruction:
mov r0, #'\r'
mov r1, '\r'
$ arm-linux-as -o /dev/null t.s
t.s: Assembler messages:
t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
The reason being that the ARM instruction set has, for a few decades
now, required the # for constants.
There are two possibilities:
1) Your binutils version is buggy, in that it now accepts constants in
ARM assembly without a preceding # and binutils needs to be fixed.
2) The change in binutils is a gratuitous change - which is a really
stupidly dumb thing to do because it means that we'll end up in this
exact situation and it breaks the established norm that has been
present for a long time.
Either way, the fact is that many binutils versions out there will not
accept the syntax that Nicolas used, and therefore the patch is broken
as far as the kernel is concerned.
So, as far as ARM assembly in the Linux kernel goes, all constants must
be preceded by # whether or not binutils requires it - no exceptions.
Please always test assembly changes with a binutils version that is not
gratuitously broken!
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 11:20 ` Russell King - ARM Linux
@ 2017-11-02 11:28 ` Chris Brandt
2017-11-02 13:04 ` Russell King - ARM Linux
2017-11-02 15:04 ` Nicolas Pitre
0 siblings, 2 replies; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 11:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017, Russell King - ARM Linux wrote:
> The compiler is only involved for the C pre-processor front-end. It's
> not involved in parsing the resulting assembly - as far as gcc is
> concerned, it could be forth in the post-processed file.
>
> GCC will then pass the post-processed output to binutils 'as' to do the
> actual assembly, and that's what should complain.
Oops, I meant to show this:
$ arm-linux-gnueabihf-as -version
GNU assembler (Linaro_Binutils-2017.01) 2.25.2 Linaro 2016_02
Copyright (C) 2014 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `arm-linux-gnueabihf'.
> So, as far as ARM assembly in the Linux kernel goes, all constants must
> be preceded by # whether or not binutils requires it - no exceptions.
> Please always test assembly changes with a binutils version that is not
> gratuitously broken!
Somewhat ironic since Nicolas works for Linaro.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 11:28 ` Chris Brandt
@ 2017-11-02 13:04 ` Russell King - ARM Linux
2017-11-02 15:04 ` Nicolas Pitre
1 sibling, 0 replies; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 13:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 11:28:55AM +0000, Chris Brandt wrote:
> On Thursday, November 02, 2017, Russell King - ARM Linux wrote:
> > So, as far as ARM assembly in the Linux kernel goes, all constants must
> > be preceded by # whether or not binutils requires it - no exceptions.
> > Please always test assembly changes with a binutils version that is not
> > gratuitously broken!
>
> Somewhat ironic since Nicolas works for Linaro.
Sorry, I don't see the connection.
As Nicolas has already admitted, he didn't actually test the patch
because the code was configured out of his test build.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 11:28 ` Chris Brandt
2017-11-02 13:04 ` Russell King - ARM Linux
@ 2017-11-02 15:04 ` Nicolas Pitre
2017-11-02 15:18 ` Robin Murphy
2017-11-02 15:23 ` Chris Brandt
1 sibling, 2 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 15:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Chris Brandt wrote:
> Oops, I meant to show this:
>
> $ arm-linux-gnueabihf-as -version
> GNU assembler (Linaro_Binutils-2017.01) 2.25.2 Linaro 2016_02
> Copyright (C) 2014 Free Software Foundation, Inc.
> This program is free software; you may redistribute it under the terms of
> the GNU General Public License version 3 or later.
> This program has absolutely no warranty.
> This assembler was configured for a target of `arm-linux-gnueabihf'.
This is very strange. Here's a quick test with all the binutils versions
I have lying around on my system:
$ echo -e "mov r0, #'\\\r'\nmov r1, '\\\r'"
mov r0, #'\r'
mov r1, '\r'
$ echo -e "mov r0, #'\\\r'\nmov r1, '\\\r'" > /tmp/t.s
$ for as in $(find /opt -name arm-linux-\*-as); do \
> $as --version | head -1; \
> $as /tmp/t.s -o /tmp/t.o; done
GNU assembler (Linaro_Binutils-2017.05) 2.27.0.20161019
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
GNU assembler (crosstool-NG linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) 2.23.1
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
GNU assembler (Linaro_Binutils-2017.05) 2.25.2 Linaro 2016_02
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
GNU assembler (crosstool-NG linaro-1.13.1-4.9-2014.09 - Linaro GCC 4.9-2014.09) 2.24.0.20140829 Linaro 2014.09
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
GNU assembler (GNU Binutils) Linaro 2014.11-3-git 2.24.0.20141017
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
GNU assembler (Linaro_Binutils-2017.08) 2.28.2.20170706
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
They all fail, including the version that looks like the one you have.
Could you try that little test above on your side?
> > So, as far as ARM assembly in the Linux kernel goes, all constants must
> > be preceded by # whether or not binutils requires it - no exceptions.
> > Please always test assembly changes with a binutils version that is not
> > gratuitously broken!
>
> Somewhat ironic since Nicolas works for Linaro.
I'm not involved with the toolchain people though, other than using
their output.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 11:09 ` Russell King - ARM Linux
@ 2017-11-02 15:12 ` Nicolas Pitre
0 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 15:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Russell King - ARM Linux wrote:
> So, in short, my tree is now closed in expectation of 4.14 on Sunday.
>
> However, if you want me to remove "8705/1: early_printk: use printascii()
> rather than printch()" I can do that, but only if you tell me before
> 4pm UTC today.
I'd say: just leave things as they are. No point disturbing your tree
again now. The downside is only cosmetic during the early boot when
early_printk is enabled. That can be fixed in v4.15-rc1.
> If we get 4.14-rc8 instead, I'll re-open for a few days for this.
Sure.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:04 ` Nicolas Pitre
@ 2017-11-02 15:18 ` Robin Murphy
2017-11-02 15:22 ` Russell King - ARM Linux
2017-11-02 15:25 ` Chris Brandt
2017-11-02 15:23 ` Chris Brandt
1 sibling, 2 replies; 45+ messages in thread
From: Robin Murphy @ 2017-11-02 15:18 UTC (permalink / raw)
To: linux-arm-kernel
On 02/11/17 15:04, Nicolas Pitre wrote:
> On Thu, 2 Nov 2017, Chris Brandt wrote:
>
>> Oops, I meant to show this:
>>
>> $ arm-linux-gnueabihf-as -version
>> GNU assembler (Linaro_Binutils-2017.01) 2.25.2 Linaro 2016_02
>> Copyright (C) 2014 Free Software Foundation, Inc.
>> This program is free software; you may redistribute it under the terms of
>> the GNU General Public License version 3 or later.
>> This program has absolutely no warranty.
>> This assembler was configured for a target of `arm-linux-gnueabihf'.
>
> This is very strange. Here's a quick test with all the binutils versions
> I have lying around on my system:
>
> $ echo -e "mov r0, #'\\\r'\nmov r1, '\\\r'"
> mov r0, #'\r'
> mov r1, '\r'
> $ echo -e "mov r0, #'\\\r'\nmov r1, '\\\r'" > /tmp/t.s
> $ for as in $(find /opt -name arm-linux-\*-as); do \
>> $as --version | head -1; \
>> $as /tmp/t.s -o /tmp/t.o; done
> GNU assembler (Linaro_Binutils-2017.05) 2.27.0.20161019
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
> GNU assembler (crosstool-NG linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) 2.23.1
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
> GNU assembler (Linaro_Binutils-2017.05) 2.25.2 Linaro 2016_02
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
> GNU assembler (crosstool-NG linaro-1.13.1-4.9-2014.09 - Linaro GCC 4.9-2014.09) 2.24.0.20140829 Linaro 2014.09
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
> GNU assembler (GNU Binutils) Linaro 2014.11-3-git 2.24.0.20141017
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
> GNU assembler (Linaro_Binutils-2017.08) 2.28.2.20170706
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
>
> They all fail, including the version that looks like the one you have.
Note that for UAL syntax, GAS follows the ARM ARM recommendation and
considers the '#' on immediate values optional everywhere. I'm going to
hazard a guess that Chris might be building a Thumb-2 kernel or somehow
otherwise has ARM_ASM_UNIFIED turned on.
Robin.
> Could you try that little test above on your side?
>
>>> So, as far as ARM assembly in the Linux kernel goes, all constants must
>>> be preceded by # whether or not binutils requires it - no exceptions.
>>> Please always test assembly changes with a binutils version that is not
>>> gratuitously broken!
>>
>> Somewhat ironic since Nicolas works for Linaro.
>
> I'm not involved with the toolchain people though, other than using
> their output.
>
>
> Nicolas
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:18 ` Robin Murphy
@ 2017-11-02 15:22 ` Russell King - ARM Linux
2017-11-02 15:25 ` Chris Brandt
1 sibling, 0 replies; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 15:22 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 03:18:11PM +0000, Robin Murphy wrote:
> Note that for UAL syntax, GAS follows the ARM ARM recommendation and
> considers the '#' on immediate values optional everywhere. I'm going to
> hazard a guess that Chris might be building a Thumb-2 kernel or somehow
> otherwise has ARM_ASM_UNIFIED turned on.
I hope people can see what a silly idea that is from the amount of
problems this is causing.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:04 ` Nicolas Pitre
2017-11-02 15:18 ` Robin Murphy
@ 2017-11-02 15:23 ` Chris Brandt
2017-11-02 15:35 ` Nicolas Pitre
2017-11-02 15:48 ` Russell King - ARM Linux
1 sibling, 2 replies; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 15:23 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> > > So, as far as ARM assembly in the Linux kernel goes, all constants
> must
> > > be preceded by # whether or not binutils requires it - no exceptions.
> > > Please always test assembly changes with a binutils version that is
> not
> > > gratuitously broken!
> >
> > Somewhat ironic since Nicolas works for Linaro.
>
> I'm not involved with the toolchain people though, other than using
> their output.
That was the irony!
As in...
Even if you built the code, you would have probably used a Linaro
toolchain and it would have worked like on my system.
Forget it. (mailing lists are so dry when it comes to humor)
> They all fail, including the version that looks like the one you have.
>
> Could you try that little test above on your side?
Yes, I also get a failure:
arm-linux-gnueabihf-as /tmp/t.s -o /tmp/t.o
/tmp/t.s: Assembler messages:
/tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
But I think the answer is not that simple.
You have no command line options.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:18 ` Robin Murphy
2017-11-02 15:22 ` Russell King - ARM Linux
@ 2017-11-02 15:25 ` Chris Brandt
1 sibling, 0 replies; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 15:25 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017 1, Robin Murphy wrote:
> >
> > They all fail, including the version that looks like the one you have.
>
> Note that for UAL syntax, GAS follows the ARM ARM recommendation and
> considers the '#' on immediate values optional everywhere. I'm going to
> hazard a guess that Chris might be building a Thumb-2 kernel or somehow
> otherwise has ARM_ASM_UNIFIED turned on.
Correct.
$ grep THUMB2 .config
CONFIG_THUMB2_KERNEL=y
$ grep UNIFIED .config
CONFIG_ARM_ASM_UNIFIED=y
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:23 ` Chris Brandt
@ 2017-11-02 15:35 ` Nicolas Pitre
2017-11-02 16:06 ` Nicolas Pitre
2017-11-02 15:48 ` Russell King - ARM Linux
1 sibling, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 15:35 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Chris Brandt wrote:
> On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> > > > So, as far as ARM assembly in the Linux kernel goes, all constants
> > must
> > > > be preceded by # whether or not binutils requires it - no exceptions.
> > > > Please always test assembly changes with a binutils version that is
> > not
> > > > gratuitously broken!
> > >
> > > Somewhat ironic since Nicolas works for Linaro.
> >
> > I'm not involved with the toolchain people though, other than using
> > their output.
>
> That was the irony!
>
> As in...
> Even if you built the code, you would have probably used a Linaro
> toolchain and it would have worked like on my system.
Thing is... I *did* test it after I figured out I needed to turn off
semihosting support. And the build failed.
> Forget it. (mailing lists are so dry when it comes to humor)
Life is tough.
> > They all fail, including the version that looks like the one you have.
> >
> > Could you try that little test above on your side?
>
> Yes, I also get a failure:
>
> arm-linux-gnueabihf-as /tmp/t.s -o /tmp/t.o
> /tmp/t.s: Assembler messages:
> /tmp/t.s:2: Error: immediate expression requires a # prefix -- `mov r1,13'
>
> But I think the answer is not that simple.
> You have no command line options.
Would be good to figure out what option makes it accept no # and see if
that can be avoided for kernel build.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:23 ` Chris Brandt
2017-11-02 15:35 ` Nicolas Pitre
@ 2017-11-02 15:48 ` Russell King - ARM Linux
2017-11-02 16:30 ` Chris Brandt
1 sibling, 1 reply; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 15:48 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 03:23:14PM +0000, Chris Brandt wrote:
> On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> > > > So, as far as ARM assembly in the Linux kernel goes, all constants
> > must
> > > > be preceded by # whether or not binutils requires it - no exceptions.
> > > > Please always test assembly changes with a binutils version that is
> > not
> > > > gratuitously broken!
> > >
> > > Somewhat ironic since Nicolas works for Linaro.
> >
> > I'm not involved with the toolchain people though, other than using
> > their output.
>
> That was the irony!
>
> As in...
> Even if you built the code, you would have probably used a Linaro
> toolchain and it would have worked like on my system.
>
> Forget it. (mailing lists are so dry when it comes to humor)
It's not mailing lists, it's email. Email lacks the facial expressions,
and the voice inflections and tone necessary to convey this extra
"metadata". It's a well known problem. Email is dry and devoid of the
subtle hints that humans need to effectively communicate. Communication
is *not* just about the words on a page.
It's why we have smilies and emojis, as an attempt to fill that void, but
I bet most of us (me included) don't use them enough.
http://www.youmeworks.com/no_honking.html
The 44% figure in that URL, if correct, is shocking. It probably means
that approaching half of all emails on this list are misconstrued by
the reader(s) of them!
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:35 ` Nicolas Pitre
@ 2017-11-02 16:06 ` Nicolas Pitre
2017-11-02 16:38 ` Chris Brandt
0 siblings, 1 reply; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 16:06 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Nicolas Pitre wrote:
> Would be good to figure out what option makes it accept no # and see if
> that can be avoided for kernel build.
OK.. I wanted to get to the bottom of this. The gas documentation says:
|9.4.2.1 Instruction Set Syntax
|..............................
|
|Two slightly different syntaxes are supported for ARM and THUMB
|instructions. The default, `divided', uses the old style where ARM and
|THUMB instructions had their own, separate syntaxes. The new,
|`unified' syntax, which can be selected via the `.syntax' directive,
|and has the following main features:
|
| * Immediate operands do not require a `#' prefix.
|
| * The `IT' instruction may appear, and if it does it is validated
| against subsequent conditional affixes. In ARM mode it does not
| generate machine code, in THUMB mode it does.
|
| * For ARM instructions the conditional affixes always appear at the
| end of the instruction. For THUMB instructions conditional
| affixes can be used, but only inside the scope of an `IT'
| instruction.
|
| * All of the instructions new to the V6T2 architecture (and later)
| are available. (Only a few such instructions can be written in the
| `divided' syntax).
|
| * The `.N' and `.W' suffixes are recognized and honored.
|
| * All instructions set the flags if and only if they have an `s'
| affix.
So this is a mixed bag of features and, unless I'm missing something,
there is no way to get some and not the others. And we do need most of
those features for Thumb2 kernel build.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 15:48 ` Russell King - ARM Linux
@ 2017-11-02 16:30 ` Chris Brandt
0 siblings, 0 replies; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 16:30 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017 1, Russell King - ARM Linux wrote:
> > Forget it. (mailing lists are so dry when it comes to humor)
>
> It's not mailing lists, it's email. Email lacks the facial expressions,
> and the voice inflections and tone necessary to convey this extra
> "metadata". It's a well known problem. Email is dry and devoid of the
> subtle hints that humans need to effectively communicate. Communication
> is *not* just about the words on a page.
Ya I know (insert Frowning Face U+2639 here)
> It's why we have smilies and emojis, as an attempt to fill that void, but
> I bet most of us (me included) don't use them enough.
I bet people would take their patches being rejected much better if you
just sent back a Pile of Poo emoji (U+1F4A9) instead.
(yes, said in a sarcastic voice)
> http://www.youmeworks.com/no_honking.html
* Don't use e-mail for emotional or sensitive topics. Pick
up the phone or visit in person.
I look forward to seeing you at the next ELC!
(yes, sarcasm again....)
Cheers
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 16:06 ` Nicolas Pitre
@ 2017-11-02 16:38 ` Chris Brandt
2017-11-02 17:10 ` Russell King - ARM Linux
0 siblings, 1 reply; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 16:38 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> OK.. I wanted to get to the bottom of this. The gas documentation says:
>
> |9.4.2.1 Instruction Set Syntax
> |..............................
> |
> |Two slightly different syntaxes are supported for ARM and THUMB
> |instructions. The default, `divided', uses the old style where ARM and
> |THUMB instructions had their own, separate syntaxes. The new,
> |`unified' syntax, which can be selected via the `.syntax' directive,
> |and has the following main features:
> |
> | * Immediate operands do not require a `#' prefix.
Well there you go!
We all have "gratuitously broken binutils"
;)
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 16:38 ` Chris Brandt
@ 2017-11-02 17:10 ` Russell King - ARM Linux
2017-11-02 17:20 ` Nicolas Pitre
` (2 more replies)
0 siblings, 3 replies; 45+ messages in thread
From: Russell King - ARM Linux @ 2017-11-02 17:10 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 02, 2017 at 04:38:11PM +0000, Chris Brandt wrote:
> On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> > OK.. I wanted to get to the bottom of this. The gas documentation says:
> >
> > |9.4.2.1 Instruction Set Syntax
> > |..............................
> > |
> > |Two slightly different syntaxes are supported for ARM and THUMB
> > |instructions. The default, `divided', uses the old style where ARM and
> > |THUMB instructions had their own, separate syntaxes. The new,
> > |`unified' syntax, which can be selected via the `.syntax' directive,
> > |and has the following main features:
> > |
> > | * Immediate operands do not require a `#' prefix.
>
> Well there you go!
>
> We all have "gratuitously broken binutils"
>
> ;)
Yes!
What reason could there be to drop the well established norm of
prefixing constants with "#" in ARM assembly, other than maybe
political pressure?
As I've already pointed out, we can see that this causes problems,
and what it means is that people now must test their changes with
Thumb2 support disabled in the kernel for there to be any valid
testing of assembly. That basically means I can't trust anyone
elses testing of patches that contain assembly, because I don't
know what configuration they've tested.
This is very bad, and it's going to make it slower to get such
patches into the kernel.
That's an unintended side effect of what was probably thought to be
a trivial decision by the ARM ISA team, but it unfortunately has
wider effects than they could have imagined.
There is another solution to this: I augment the patch system with an
ARM assembly parser that detects this before it gets accepted,
rejecting patches that omit the # for constants. However, that is
incomplete, because we now live in a world where ARM assembly gets
added to the kernel via many different git trees.
Basically, this change in the ARM syntax should never have been made.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 17:10 ` Russell King - ARM Linux
@ 2017-11-02 17:20 ` Nicolas Pitre
2017-11-02 17:28 ` Chris Brandt
2017-11-02 18:29 ` Robin Murphy
2 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 17:20 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Russell King - ARM Linux wrote:
> On Thu, Nov 02, 2017 at 04:38:11PM +0000, Chris Brandt wrote:
> > On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
> > > OK.. I wanted to get to the bottom of this. The gas documentation says:
> > >
> > > |9.4.2.1 Instruction Set Syntax
> > > |..............................
> > > |
> > > |Two slightly different syntaxes are supported for ARM and THUMB
> > > |instructions. The default, `divided', uses the old style where ARM and
> > > |THUMB instructions had their own, separate syntaxes. The new,
> > > |`unified' syntax, which can be selected via the `.syntax' directive,
> > > |and has the following main features:
> > > |
> > > | * Immediate operands do not require a `#' prefix.
> >
> > Well there you go!
> >
> > We all have "gratuitously broken binutils"
> >
> > ;)
>
> Yes!
>
> What reason could there be to drop the well established norm of
> prefixing constants with "#" in ARM assembly, other than maybe
> political pressure?
Dunno. But I wouldn't mind it at all if such a "feature" was selectable
*separately* from the others, or if it could be opted out from the
unified syntax. As it is I don't see how to achieve that.
> There is another solution to this: I augment the patch system with an
> ARM assembly parser that detects this before it gets accepted,
> rejecting patches that omit the # for constants. However, that is
> incomplete, because we now live in a world where ARM assembly gets
> added to the kernel via many different git trees.
I'd go for the ".syntax require_pound_literals" addition to the
assembler. That will make the solution available to everyone
eventually.
Nicolas
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 17:10 ` Russell King - ARM Linux
2017-11-02 17:20 ` Nicolas Pitre
@ 2017-11-02 17:28 ` Chris Brandt
2017-11-02 18:29 ` Robin Murphy
2 siblings, 0 replies; 45+ messages in thread
From: Chris Brandt @ 2017-11-02 17:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, November 02, 2017, Russell King - ARM Linux wrote:
> This is very bad, and it's going to make it slower to get such
> patches into the kernel.
Well crap! All I wanted was my carriage returns back.
This is going to come back and bite me in the ass later when I try to
submit some XIP code changes in head.S that I've been meaning to get to.
Chris
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 17:10 ` Russell King - ARM Linux
2017-11-02 17:20 ` Nicolas Pitre
2017-11-02 17:28 ` Chris Brandt
@ 2017-11-02 18:29 ` Robin Murphy
2017-11-02 21:46 ` syntax unified, was " Nicolas Pitre
2 siblings, 1 reply; 45+ messages in thread
From: Robin Murphy @ 2017-11-02 18:29 UTC (permalink / raw)
To: linux-arm-kernel
On 02/11/17 17:10, Russell King - ARM Linux wrote:
> On Thu, Nov 02, 2017 at 04:38:11PM +0000, Chris Brandt wrote:
>> On Thursday, November 02, 2017 1, Nicolas Pitre wrote:
>>> OK.. I wanted to get to the bottom of this. The gas documentation says:
>>>
>>> |9.4.2.1 Instruction Set Syntax
>>> |..............................
>>> |
>>> |Two slightly different syntaxes are supported for ARM and THUMB
>>> |instructions. The default, `divided', uses the old style where ARM and
>>> |THUMB instructions had their own, separate syntaxes. The new,
>>> |`unified' syntax, which can be selected via the `.syntax' directive,
>>> |and has the following main features:
>>> |
>>> | * Immediate operands do not require a `#' prefix.
>>
>> Well there you go!
>>
>> We all have "gratuitously broken binutils"
>>
>> ;)
>
> Yes!
>
> What reason could there be to drop the well established norm of
> prefixing constants with "#" in ARM assembly, other than maybe
> political pressure?
>
> As I've already pointed out, we can see that this causes problems,
> and what it means is that people now must test their changes with
> Thumb2 support disabled in the kernel for there to be any valid
> testing of assembly. That basically means I can't trust anyone
> elses testing of patches that contain assembly, because I don't
> know what configuration they've tested.
But that's already been true since the introduction of THUMB2_KERNEL -
there are instructions which exist in ARM but not in Thumb, and vice
versa; there are constants which can be encoded in Thumb, but not in
ARM; in general this syntactic difference doesn't really add anything
other than being perhaps slightly easier to fall foul of.
> This is very bad, and it's going to make it slower to get such
> patches into the kernel.
>
> That's an unintended side effect of what was probably thought to be
> a trivial decision by the ARM ISA team, but it unfortunately has
> wider effects than they could have imagined.
>
> There is another solution to this: I augment the patch system with an
> ARM assembly parser that detects this before it gets accepted,
> rejecting patches that omit the # for constants. However, that is
> incomplete, because we now live in a world where ARM assembly gets
> added to the kernel via many different git trees.
Or we could just enable unified syntax by default. AFAICT, binutils has
supported UAL for over 12 years now, and the minimal supported version
of 2.20 quoted in Documentation/process/ is considerably more recent
than that.
> Basically, this change in the ARM syntax should never have been made.
Careful what you wish for - if GAS should be strict about unified syntax
and not take an allowable implementation option, it should definitely be
strict about legacy syntax and not accept UAL mnemonics which don't even
exist in the old language. Then we'd have much bigger problems for
antique toolchains ;)
Robin.
^ permalink raw reply [flat|nested] 45+ messages in thread
* syntax unified, was Re: [PATCH 3/3] ARM: early_printk: use printascii() rather than printch()
2017-11-02 18:29 ` Robin Murphy
@ 2017-11-02 21:46 ` Nicolas Pitre
0 siblings, 0 replies; 45+ messages in thread
From: Nicolas Pitre @ 2017-11-02 21:46 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2 Nov 2017, Robin Murphy wrote:
> On 02/11/17 17:10, Russell King - ARM Linux wrote:
> > There is another solution to this: I augment the patch system with an
> > ARM assembly parser that detects this before it gets accepted,
> > rejecting patches that omit the # for constants. However, that is
> > incomplete, because we now live in a world where ARM assembly gets
> > added to the kernel via many different git trees.
>
> Or we could just enable unified syntax by default. AFAICT, binutils has
> supported UAL for over 12 years now, and the minimal supported version
> of 2.20 quoted in Documentation/process/ is considerably more recent
> than that.
You have a point here. So let's see in details...
Documentation/process/changes.rst sais:
|Upgrade to at **least** these software revisions before thinking you've
|encountered a bug! If you're unsure what version you're currently
|running, the suggested command should tell you.
[...]
|GNU C 3.2 gcc --version
|GNU make 3.81 make --version
|binutils 2.20 ld -v
[...]
On ARM we enforce these additional restrictions in
arch/arm/kernel/asm-offsets.c:
|/*
| * GCC 3.0, 3.1: general bad code generation.
| * GCC 3.2.0: incorrect function argument offset calculation.
| * GCC 3.2.x: miscompiles NEW_AUX_ENT in fs/binfmt_elf.c
| * (http://gcc.gnu.org/PR8896) and incorrect structure
| * initialisation in fs/jffs2/erase.c
| * GCC 4.8.0-4.8.2: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58854
| * miscompiles find_get_entry(), and can result in EXT3 and EXT4
| * filesystem corruption (possibly other FS too).
| */
|#ifdef __GNUC__
|#if (__GNUC__ == 3 && __GNUC_MINOR__ < 3)
|#error Your compiler is too buggy; it is known to miscompile kernels.
|#error Known good compilers: 3.3, 4.x
|#endif
[...]
It should be quite safe to assume that any gcc-3.3 installations came
with@least binutils-2.20.
So let's try that out:
binutils-2.20.1]$ ./gas/as-new --version
GNU assembler (GNU Binutils) 2.20.1.20100303
[...]
binutils-2.20.1]$ echo "mov r0, '\r'" | ./gas/as-new - -o /tmp/t.o
{standard input}: Assembler messages:
{standard input}:1: Error: immediate expression requires a # prefix -- `mov r0,13'
binutils-2.20.1]$ echo ".syntax unified; mov r0, '\r'" | ./gas/as-new - -o /tmp/t.o
(nothing)
binutils-2.20.1]$ ./binutils/objdump -d /tmp/t.o
/tmp/t.o: file format elf32-littlearm
Disassembly of section .text:
00000000 <.text>:
0: e3a0000d mov r0, #13
Digging into the binutils git repository, it looks like the unified
syntax was indeed supported in 2005.
Therefore every setup capable of compiling the latest linux kernel for
ARM should be ".syntax unified" ready as Robin said.
So... I think the best thing to do at this point is to enable the
unified syntax unconditionally as suggested. This even has the side
effect of removing a bunch of macros that cause trouble with LTO builds.
I propose the following patch:
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 7888c9803e..de1dd6e9cf 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1531,12 +1531,10 @@ config THUMB2_KERNEL
bool "Compile the kernel in Thumb-2 mode" if !CPU_THUMBONLY
depends on (CPU_V7 || CPU_V7M) && !CPU_V6 && !CPU_V6K
default y if CPU_THUMBONLY
- select ARM_ASM_UNIFIED
select ARM_UNWIND
help
By enabling this option, the kernel will be compiled in
- Thumb-2 mode. A compiler/assembler that understand the unified
- ARM-Thumb syntax is needed.
+ Thumb-2 mode.
If unsure, say N.
@@ -1571,9 +1569,6 @@ config THUMB2_AVOID_R_ARM_THM_JUMP11
Unless you are sure your tools don't have this problem, say Y.
-config ARM_ASM_UNIFIED
- bool
-
config ARM_PATCH_IDIV
bool "Runtime patch udiv/sdiv instructions into __aeabi_{u}idiv()"
depends on CPU_32v7 && !XIP_KERNEL
diff --git a/arch/arm/include/asm/unified.h b/arch/arm/include/asm/unified.h
index a91ae49961..2c3b952be6 100644
--- a/arch/arm/include/asm/unified.h
+++ b/arch/arm/include/asm/unified.h
@@ -20,8 +20,10 @@
#ifndef __ASM_UNIFIED_H
#define __ASM_UNIFIED_H
-#if defined(__ASSEMBLY__) && defined(CONFIG_ARM_ASM_UNIFIED)
+#if defined(__ASSEMBLY__)
.syntax unified
+#else
+__asm__(".syntax unified");
#endif
#ifdef CONFIG_CPU_V7M
@@ -64,77 +66,4 @@
#endif /* CONFIG_THUMB2_KERNEL */
-#ifndef CONFIG_ARM_ASM_UNIFIED
-
-/*
- * If the unified assembly syntax isn't used (in ARM mode), these
- * macros expand to an empty string
- */
-#ifdef __ASSEMBLY__
- .macro it, cond
- .endm
- .macro itt, cond
- .endm
- .macro ite, cond
- .endm
- .macro ittt, cond
- .endm
- .macro itte, cond
- .endm
- .macro itet, cond
- .endm
- .macro itee, cond
- .endm
- .macro itttt, cond
- .endm
- .macro ittte, cond
- .endm
- .macro ittet, cond
- .endm
- .macro ittee, cond
- .endm
- .macro itett, cond
- .endm
- .macro itete, cond
- .endm
- .macro iteet, cond
- .endm
- .macro iteee, cond
- .endm
-#else /* !__ASSEMBLY__ */
-__asm__(
-" .macro it, cond\n"
-" .endm\n"
-" .macro itt, cond\n"
-" .endm\n"
-" .macro ite, cond\n"
-" .endm\n"
-" .macro ittt, cond\n"
-" .endm\n"
-" .macro itte, cond\n"
-" .endm\n"
-" .macro itet, cond\n"
-" .endm\n"
-" .macro itee, cond\n"
-" .endm\n"
-" .macro itttt, cond\n"
-" .endm\n"
-" .macro ittte, cond\n"
-" .endm\n"
-" .macro ittet, cond\n"
-" .endm\n"
-" .macro ittee, cond\n"
-" .endm\n"
-" .macro itett, cond\n"
-" .endm\n"
-" .macro itete, cond\n"
-" .endm\n"
-" .macro iteet, cond\n"
-" .endm\n"
-" .macro iteee, cond\n"
-" .endm\n");
-#endif /* __ASSEMBLY__ */
-
-#endif /* CONFIG_ARM_ASM_UNIFIED */
-
#endif /* !__ASM_UNIFIED_H */
^ permalink raw reply related [flat|nested] 45+ messages in thread
end of thread, other threads:[~2017-11-02 21:46 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-02 2:06 [PATCH 0/3] a few debug_ll fixes and improvements Nicolas Pitre
2017-10-02 2:06 ` [PATCH 1/3] ARM: debug.S: move hexbuf to a writable section Nicolas Pitre
2017-10-02 2:06 ` [PATCH 2/3] ARM: semihosting: use proper instruction on v7m processors Nicolas Pitre
2017-10-02 2:06 ` [PATCH 3/3] ARM: early_printk: use printascii() rather than printch() Nicolas Pitre
2017-10-02 5:20 ` Uwe Kleine-König
2017-10-02 14:09 ` Nicolas Pitre
2017-10-31 11:28 ` Chris Brandt
2017-10-31 16:22 ` Nicolas Pitre
2017-10-31 16:38 ` Robin Murphy
2017-10-31 17:06 ` Nicolas Pitre
2017-10-31 17:16 ` Russell King - ARM Linux
2017-10-31 17:48 ` Nicolas Pitre
2017-10-31 17:53 ` Russell King - ARM Linux
2017-10-31 18:15 ` Nicolas Pitre
2017-10-31 18:20 ` Russell King - ARM Linux
2017-10-31 18:35 ` Nicolas Pitre
2017-10-31 19:12 ` Chris Brandt
2017-10-31 19:28 ` Nicolas Pitre
2017-10-31 21:50 ` Russell King - ARM Linux
2017-10-31 23:35 ` Nicolas Pitre
2017-10-31 23:50 ` Russell King - ARM Linux
2017-11-02 0:09 ` Russell King - ARM Linux
2017-11-02 3:59 ` Nicolas Pitre
2017-11-02 4:16 ` Nicolas Pitre
2017-11-02 11:09 ` Russell King - ARM Linux
2017-11-02 15:12 ` Nicolas Pitre
2017-11-02 11:06 ` Chris Brandt
2017-11-02 11:20 ` Russell King - ARM Linux
2017-11-02 11:28 ` Chris Brandt
2017-11-02 13:04 ` Russell King - ARM Linux
2017-11-02 15:04 ` Nicolas Pitre
2017-11-02 15:18 ` Robin Murphy
2017-11-02 15:22 ` Russell King - ARM Linux
2017-11-02 15:25 ` Chris Brandt
2017-11-02 15:23 ` Chris Brandt
2017-11-02 15:35 ` Nicolas Pitre
2017-11-02 16:06 ` Nicolas Pitre
2017-11-02 16:38 ` Chris Brandt
2017-11-02 17:10 ` Russell King - ARM Linux
2017-11-02 17:20 ` Nicolas Pitre
2017-11-02 17:28 ` Chris Brandt
2017-11-02 18:29 ` Robin Murphy
2017-11-02 21:46 ` syntax unified, was " Nicolas Pitre
2017-11-02 15:48 ` Russell King - ARM Linux
2017-11-02 16:30 ` Chris Brandt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.