[U-Boot] [PATCH v1 4/5] bootcount: display5: spl: Extend SPL to support bootcount checking

[Stefan Roese]

Hi Lukasz,

On 26.02.2018 16:31, Lukasz Majewski wrote:
> On Mon, 26 Feb 2018 16:22:25 +0100
> Lukasz Majewski <lukma at denx.de> wrote:
> 
>> Hi Stefan,
>>
>>> Hi Lukasz,
>>>
>>> On 26.02.2018 13:22, Lukasz Majewski wrote:
>>>> This patch is necessary for providing basic bootcount checking in
>>>> the case of using "falcon" boot mode.
>>>>
>>>> It forces u-boot proper boot, when we exceed the number of errors.
>>>>
>>>> Signed-off-by: Lukasz Majewski <lukma at denx.de>
>>>> ---
>>>>
>>>>    board/liebherr/display5/spl.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/board/liebherr/display5/spl.c
>>>> b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc
>>>> 100644 --- a/board/liebherr/display5/spl.c
>>>> +++ b/board/liebherr/display5/spl.c
>>>> @@ -20,6 +20,7 @@
>>>>    #include <environment.h>
>>>>    #include <fsl_esdhc.h>
>>>>    #include <netdev.h>
>>>> +#include <bootcount.h>
>>>>    #include "common.h"
>>>>    
>>>>    DECLARE_GLOBAL_DATA_PTR;
>>>> @@ -194,6 +195,9 @@ void board_init_f(ulong dummy)
>>>>    	/* Clear the BSS. */
>>>>    	memset(__bss_start, 0, __bss_end - __bss_start);
>>>>    
>>>> +	/* Increment bootcount */
>>>> +	bootcount_inc();
>>>> +
>>>
>>> Wouldn't it be better, to move this incrementing into some common
>>> code, so that other platform who might use the bootcounter in SPL
>>> in the future could also use it without code duplication?
>>>
>>> What do you think?
>>
>> To provide this functionality I need to add bootcount_inc() and check
>> bootcount_error(). Both are static inlines from  #include
>> <bootcount.h>
>>
>> However, maybe it would be better to put this code to generic SPL. I
>> will try to do it for v2.
> 
> The problem here is that we need very early code to do this (increment
> bootcount). Now it is done in SPL's board_init_f.
> It is the first C function, called from crt0.S asm file.

And why does it need to be done "very early" in the SPL? In U-Boot
proper, its not done very early but only after all is initialized -
so quite late in the boot process. Why should this be different for
SPL? Why not increment the bootcounter somewhere in board_init_r()
(common/spl/spl.c)?

> I'm not sure if we have such early code common for all archs/platforms.
> 
> If we put this bootcount_inc() to latter SPL code, we risk hanging in
> SPL with WDT reset and not incremented bootcount...

So you have a very fast watchdog on your system?

> Also the bootcount shall be incremented in the same function where we
> enable and initialize WDT.

Hmmm. This is also different from U-Boot proper, AFAIK. Where is your
watchdog now initialized in SPL? You should be okay, if the watchdog
is enabled late in the SPL boot stage. Or am I missing something
here?

Thanks,
Stefan