[PATCH 3/3] ARM: early_printk: use printascii() rather than printch()

[Russell King - ARM Linux]

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