Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 02/13] openrisc: Cleanup emergency print handling
To:     Stafford Horne <shorne@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Jonas Bonn <jonas@southpole.se>,
        Openrisc <openrisc@lists.librecores.org>,
        Randy Dunlap <rdunlap@infradead.org>
References: <20220515124158.3167452-1-shorne@gmail.com>
 <20220515124158.3167452-3-shorne@gmail.com>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <7b18962e-d1ce-699f-dd59-719255f19ade@sholland.org>
Date:   Sun, 15 May 2022 11:03:47 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20220515124158.3167452-3-shorne@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Stafford,

On 5/15/22 7:41 AM, Stafford Horne wrote:
> The emergency print support only works for 8250 compatible serial ports.
> Now that OpenRISC platforms may be configured with different serial port
> hardware we don't want emergency print to try to print to non-existent
> hardware which will cause lockups.
> 
> This patch contains several fixes to get emergency print working again:
> 
>  - Update symbol loading to not assume the location of symbols
>  - Split the putc print operation out to its own function to allow
>    for different future implementations.
>  - Update _emergency_print_nr and _emergency_print to use the putc
>    function.
>  - Guard serial 8250 specific sequences by CONFIG_SERIAL_8250
>  - Update string line feed from lf,cr to cr,lf.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>  arch/openrisc/kernel/head.S | 148 +++++++++++++++++++++---------------
>  1 file changed, 85 insertions(+), 63 deletions(-)
> 
> diff --git a/arch/openrisc/kernel/head.S b/arch/openrisc/kernel/head.S
> index 15f1b38dfe03..b1f3a65c271c 100644
> --- a/arch/openrisc/kernel/head.S
> +++ b/arch/openrisc/kernel/head.S
> @@ -297,19 +297,23 @@
>  	/* temporary store r3, r9 into r1, r10 */		;\
>  	l.addi	r1,r3,0x0					;\
>  	l.addi	r10,r9,0x0					;\
> -	/* the string referenced by r3 must be low enough */	;\
> +	LOAD_SYMBOL_2_GPR(r9,_string_unhandled_exception)	;\
> +	tophys	(r3,r9)						;\
>  	l.jal	_emergency_print				;\
> -	l.ori	r3,r0,lo(_string_unhandled_exception)		;\
> +	 l.nop							;\
>  	l.mfspr	r3,r0,SPR_NPC					;\
>  	l.jal	_emergency_print_nr				;\
> -	l.andi	r3,r3,0x1f00					;\
> -	/* the string referenced by r3 must be low enough */	;\
> +	 l.andi	r3,r3,0x1f00					;\
> +	LOAD_SYMBOL_2_GPR(r9,_string_epc_prefix)		;\
> +	tophys	(r3,r9)						;\
>  	l.jal	_emergency_print				;\
> -	l.ori	r3,r0,lo(_string_epc_prefix)			;\
> +	 l.nop							;\
>  	l.jal	_emergency_print_nr				;\
> -	l.mfspr	r3,r0,SPR_EPCR_BASE				;\
> +	 l.mfspr r3,r0,SPR_EPCR_BASE				;\
> +	LOAD_SYMBOL_2_GPR(r9,_string_nl)			;\
> +	tophys	(r3,r9)						;\
>  	l.jal	_emergency_print				;\
> -	l.ori	r3,r0,lo(_string_nl)				;\
> +	 l.nop							;\
>  	/* end of printing */					;\
>  	l.addi	r3,r1,0x0					;\
>  	l.addi	r9,r10,0x0					;\
> @@ -1530,65 +1534,99 @@ trampoline_out:
>  	l.jr	r9
>  	l.nop
>  
> -
>  /*
> - * DSCR: prints a string referenced by r3.
> + * DESC: Prints ASCII character stored in r7
>   *
> - * PRMS: r3     	- address of the first character of null
> - *			terminated string to be printed
> + * PRMS: r7	- a 32-bit value with an ASCII character in the first byte
> + *		position.
>   *
> - * PREQ: UART at UART_BASE_ADD has to be initialized
> + * PREQ: The UART at UART_BASE_ADD has to be initialized
>   *
> - * POST: caller should be aware that r3, r9 are changed
> + * POST: internally used but restores:
> + * 	 r4	- to store UART_BASE_ADD
> + *	 r5	- for loading OFF_TXFULL / THRE,TEMT
> + *	 r6	- for storing bitmask (SERIAL_8250)
>   */
> -ENTRY(_emergency_print)
> +ENTRY(_emergency_putc)
>  	EMERGENCY_PRINT_STORE_GPR4
>  	EMERGENCY_PRINT_STORE_GPR5
>  	EMERGENCY_PRINT_STORE_GPR6
> -	EMERGENCY_PRINT_STORE_GPR7
> -2:
> -	l.lbz	r7,0(r3)
> -	l.sfeq	r7,r0
> -	l.bf	9f
> -	l.nop
>  
> -// putc:
>  	l.movhi r4,hi(UART_BASE_ADD)
> +	l.ori	r4,r4,lo(UART_BASE_ADD)
>  
> +#elif defined(CONFIG_SERIAL_8250)

This needs to use #if in this patch (and #elif in the next patch).

Regards,
Samuel

> +	/* Check UART LSR THRE (hold) bit */
>  	l.addi  r6,r0,0x20
>  1:      l.lbz   r5,5(r4)
>  	l.andi  r5,r5,0x20
>  	l.sfeq  r5,r6
>  	l.bnf   1b
> -	l.nop
> +	 l.nop
>  
> +	/* Write character */
>  	l.sb    0(r4),r7
>  
> +	/* Check UART LSR THRE|TEMT (hold, empty) bits */
>  	l.addi  r6,r0,0x60
>  1:      l.lbz   r5,5(r4)
>  	l.andi  r5,r5,0x60
>  	l.sfeq  r5,r6
>  	l.bnf   1b
> -	l.nop
> +	 l.nop
> +#endif
> +	EMERGENCY_PRINT_LOAD_GPR6
> +	EMERGENCY_PRINT_LOAD_GPR5
> +	EMERGENCY_PRINT_LOAD_GPR4
> +	l.jr	r9
> +	 l.nop
> +
> +/*
> + * DSCR: prints a string referenced by r3.
> + *
> + * PRMS: r3     	- address of the first character of null
> + *			terminated string to be printed
> + *
> + * PREQ: UART at UART_BASE_ADD has to be initialized
> + *
> + * POST: caller should be aware that r3, r9 are changed
> + */
> +ENTRY(_emergency_print)
> +	EMERGENCY_PRINT_STORE_GPR7
> +	EMERGENCY_PRINT_STORE_GPR9
> +
> +	/* Load character to r7, check for null terminator */
> +2:	l.lbz	r7,0(r3)
> +	l.sfeqi	r7,0x0
> +	l.bf	9f
> +	 l.nop
> +
> +	l.jal	_emergency_putc
> +	 l.nop
>  
>  	/* next character */
>  	l.j	2b
> -	l.addi	r3,r3,0x1
> +	 l.addi	r3,r3,0x1
>  
>  9:
> +	EMERGENCY_PRINT_LOAD_GPR9
>  	EMERGENCY_PRINT_LOAD_GPR7
> -	EMERGENCY_PRINT_LOAD_GPR6
> -	EMERGENCY_PRINT_LOAD_GPR5
> -	EMERGENCY_PRINT_LOAD_GPR4
>  	l.jr	r9
> -	l.nop
> +	 l.nop
>  
> +/*
> + * DSCR: prints a number in r3 in hex.
> + *
> + * PRMS: r3     	- a 32-bit unsigned integer
> + *
> + * PREQ: UART at UART_BASE_ADD has to be initialized
> + *
> + * POST: caller should be aware that r3, r9 are changed
> + */
>  ENTRY(_emergency_print_nr)
> -	EMERGENCY_PRINT_STORE_GPR4
> -	EMERGENCY_PRINT_STORE_GPR5
> -	EMERGENCY_PRINT_STORE_GPR6
>  	EMERGENCY_PRINT_STORE_GPR7
>  	EMERGENCY_PRINT_STORE_GPR8
> +	EMERGENCY_PRINT_STORE_GPR9
>  
>  	l.addi	r8,r0,32		// shift register
>  
> @@ -1600,58 +1638,39 @@ ENTRY(_emergency_print_nr)
>  	/* don't skip the last zero if number == 0x0 */
>  	l.sfeqi	r8,0x4
>  	l.bf	2f
> -	l.nop
> +	 l.nop
>  
>  	l.sfeq	r7,r0
>  	l.bf	1b
> -	l.nop
> +	 l.nop
>  
>  2:
>  	l.srl	r7,r3,r8
>  
>  	l.andi	r7,r7,0xf
>  	l.sflts	r8,r0
> -	l.bf	9f
> +	 l.bf	9f
>  
> +	/* Numbers greater than 9 translate to a-f */
>  	l.sfgtui r7,0x9
>  	l.bnf	8f
> -	l.nop
> +	 l.nop
>  	l.addi	r7,r7,0x27
>  
> -8:
> -	l.addi	r7,r7,0x30
> -// putc:
> -	l.movhi r4,hi(UART_BASE_ADD)
> -
> -	l.addi  r6,r0,0x20
> -1:      l.lbz   r5,5(r4)
> -	l.andi  r5,r5,0x20
> -	l.sfeq  r5,r6
> -	l.bnf   1b
> -	l.nop
> -
> -	l.sb    0(r4),r7
> -
> -	l.addi  r6,r0,0x60
> -1:      l.lbz   r5,5(r4)
> -	l.andi  r5,r5,0x60
> -	l.sfeq  r5,r6
> -	l.bnf   1b
> -	l.nop
> +	/* Convert to ascii and output character */
> +8:	l.jal	_emergency_putc
> +	 l.addi	r7,r7,0x30
>  
>  	/* next character */
>  	l.j	2b
>  	l.addi	r8,r8,-0x4
>  
>  9:
> +	EMERGENCY_PRINT_LOAD_GPR9
>  	EMERGENCY_PRINT_LOAD_GPR8
>  	EMERGENCY_PRINT_LOAD_GPR7
> -	EMERGENCY_PRINT_LOAD_GPR6
> -	EMERGENCY_PRINT_LOAD_GPR5
> -	EMERGENCY_PRINT_LOAD_GPR4
>  	l.jr	r9
> -	l.nop
> -
> +	 l.nop
>  
>  /*
>   * This should be used for debugging only.
> @@ -1676,7 +1695,9 @@ ENTRY(_emergency_print_nr)
>  
>  ENTRY(_early_uart_init)
>  	l.movhi	r3,hi(UART_BASE_ADD)
> +	l.ori	r3,r3,lo(UART_BASE_ADD)
>  
> +#if defined(CONFIG_SERIAL_8250)
>  	l.addi	r4,r0,0x7
>  	l.sb	0x2(r3),r4
>  
> @@ -1694,9 +1715,10 @@ ENTRY(_early_uart_init)
>  	l.addi  r4,r0,((UART_DIVISOR) & 0x000000ff)
>  	l.sb	UART_DLL(r3),r4
>  	l.sb	0x3(r3),r5
> +#endif
>  
>  	l.jr	r9
> -	l.nop
> +	 l.nop
>  
>  	.align	0x1000
>  	.global _secondary_evbar
> @@ -1711,13 +1733,13 @@ _secondary_evbar:
>  
>  	.section .rodata
>  _string_unhandled_exception:
> -	.string "\n\rRunarunaround: Unhandled exception 0x\0"
> +	.string "\r\nRunarunaround: Unhandled exception 0x\0"
>  
>  _string_epc_prefix:
>  	.string ": EPC=0x\0"
>  
>  _string_nl:
> -	.string "\n\r\0"
> +	.string "\r\n\0"
>  
>  
>  /* ========================================[ page aligned structures ]=== */
>