Re: [PATCH v2 2/2] ARM: setup_c: avoid clearing BSS twice

[David Picard <david.picard@clermont.in2p3.fr>]

I tested from the last but one commit on master, but I don't think it's 
a big deal.

Anyway, my board boots. Congrats!

David

Le 17/03/2026 à 10:06, Sascha Hauer a écrit :
> Hi David,
>
> On Mon, Mar 16, 2026 at 02:21:37PM +0100, David Picard wrote:
>> Hi,
>>
>> I would like this patches be reverted, since, despite the proposed fixes, it
>> prevents the board arch/arm/boards/enclustra-sa2 to boot.
> I reverted this patch for now, but nevertheless I'd like to understand
> what's wrong with it.
>
>> With the option CONFIG_DEBUG_LL enabled, the output is:
>>
>> lowlevel init done
>> SDRAM setup...
>> SDRAM calibration...
>> done
>>
>> Then, it hangs.
> Could you apply the following patch to current master and see what the
> output is?
>
> Thanks
>   Sascha
>
> ----------------------------------8<-----------------------------------
>
>  From b784bc80756d127ae4a824ad235d4e113d5dcc1f Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Wed, 4 Mar 2026 08:53:22 +0100
> Subject: [PATCH] ARM: setup_c: avoid clearing BSS twice
>
> Add a bss_cleared variable that is set after the first call to
> setup_c(). On subsequent calls, the BSS clear is skipped to avoid
> wiping already-initialized data.
>
> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Link: https://lore.barebox.org/20260304-arm-setup-c-v2-2-d26af520ab03@pengutronix.de
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   arch/arm/cpu/setupc_32.S  | 16 ++++++++++++++--
>   arch/arm/cpu/setupc_64.S  | 15 +++++++++++++--
>   arch/arm/cpu/uncompress.c | 15 +++++++++++++++
>   3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/cpu/setupc_32.S b/arch/arm/cpu/setupc_32.S
> index fa31f94442..fadfc28ae1 100644
> --- a/arch/arm/cpu/setupc_32.S
> +++ b/arch/arm/cpu/setupc_32.S
> @@ -7,18 +7,30 @@
>   .section .text.setupc
>   
>   /*
> - * setup_c: clear bss
> + * setup_c: clear bss if not yet done
>    */
>   ENTRY(setup_c)
>   	push	{r4, lr}
> +	ldr	r0, =bss_cleared
> +	ldr	r1, [r0]
> +	cmp	r1, #0
> +	bne	1f			/* skip if already done */
>   	ldr	r0, =__bss_start
>   	mov	r1, #0
>   	ldr	r2, =__bss_stop
>   	sub	r2, r2, r0
>   	bl	__memset		/* clear bss */
> -	pop	{r4, pc}
> +	ldr	r0, =bss_cleared
> +	mov	r1, #1
> +	str	r1, [r0]		/* mark bss cleared */
> +1:	pop	{r4, pc}
>   ENDPROC(setup_c)
>   
> +.section .data.bss_cleared
> +.balign 4
> +bss_cleared:
> +	.word 	0
> +
>   /*
>    * void relocate_to_adr(unsigned long targetadr)
>    *
> diff --git a/arch/arm/cpu/setupc_64.S b/arch/arm/cpu/setupc_64.S
> index 18ab7ff3fe..a0767c5136 100644
> --- a/arch/arm/cpu/setupc_64.S
> +++ b/arch/arm/cpu/setupc_64.S
> @@ -7,19 +7,30 @@
>   .section .text.setupc
>   
>   /*
> - * setup_c: clear bss
> + * setup_c: clear bss if not yet done
>    */
>   ENTRY(setup_c)
> +	adr_l	x0, bss_cleared
> +	ldr	w1, [x0]
> +	cbnz	w1, 1f			/* skip if already done */
>   	mov	x15, x30
>   	adr_l	x0, __bss_start
>   	mov	x1, #0
>   	adr_l	x2, __bss_stop
>   	sub	x2, x2, x0
>   	bl	__memset		/* clear bss */
> +	adr_l	x0, bss_cleared
> +	mov	w1, #1
> +	str	w1, [x0]		/* mark bss cleared */
>   	mov	x30, x15
> -	ret
> +1:	ret
>   ENDPROC(setup_c)
>   
> +.section .data.bss_cleared
> +.balign 4
> +bss_cleared:
> +	.word 	0
> +
>   /*
>    * void relocate_to_adr(unsigned long targetadr)
>    *
> diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
> index 38f7dbc113..bde0f2a0a5 100644
> --- a/arch/arm/cpu/uncompress.c
> +++ b/arch/arm/cpu/uncompress.c
> @@ -50,6 +50,15 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>   	pg_start = runtime_address(input_data);
>   	pg_end = runtime_address(input_data_end);
>   
> +	puthex_ll((unsigned long)pg_start); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll((unsigned long)pg_end); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll((unsigned long)pc); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll(membase); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll(memsize); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll((unsigned long)&__bss_start); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll((unsigned long)&__bss_stop); putc_ll('\r'); putc_ll('\n');
> +	puthex_ll((unsigned long)&__image_end); putc_ll('\r'); putc_ll('\n');
> +
>   	/*
>   	 * If we run from inside the memory just relocate the binary
>   	 * to the current address. Otherwise it may be a readonly location.
> @@ -60,11 +69,17 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>   	else
>   		relocate_to_adr(membase);
>   
> +	putc_ll('A');
> +
>   	pg_len = pg_end - pg_start;
>   	uncompressed_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
>   
> +	putc_ll('B');
> +
>   	setup_c();
>   
> +	putc_ll('C');
> +
>   	pr_debug("memory at 0x%08lx, size 0x%08lx\n", membase, memsize);
>   
>   	arm_pbl_init_exceptions();