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

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

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

-- 
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 |