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

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

Hi,

The boot log with CONFIG_DEBUG_LL enabled for 1st and 2nd stage: 
https://paste.debian.net/hidden/d82f6a59

David

Le 17/03/2026 à 17:11, Ahmad Fatoum a écrit :
> Hello David,
>
> On 3/17/26 5:03 PM, David Picard wrote:
>> I applied the patch on 36b7e6f4245cb29e3e95cd9e50bc49ad61ba9c2b.
>>
>> The boot log: https://paste.debian.net/hidden/7fa6ec69
> Is it possible to enable CONFIG_DEBUG_LL also in the first stage?
>
> I assume that's what was hanging with the patch applied, so debug output
> from there would be valuable.
>
> Thanks,
> Ahmad
>
>> David
>>
>>
>> Le 17/03/2026 à 16:56, Ahmad Fatoum a écrit :
>>> 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();
>>>>
>>