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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello David,

On 3/17/26 1:43 PM, David Picard wrote:
> 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!

I think you missed Sascha's patch at the end of his mail.

The change that has been dropped from next is a building block for a
boot time optimization and it would be very helpful to understand what
exactly broke at your side, so a revised patch can be applied.

To help with that, Sascha included a patch with some extra debug output.

Cheers,
Ahmad

> 
> 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();
> 
> 
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |