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

[Lukasz Majewski <lukma@denx.de>]

Hi Stefan,

> Hi Lukasz,
> 
> On 26.02.2018 16:31, Lukasz Majewski wrote:
> > On Mon, 26 Feb 2018 16:22:25 +0100
> > Lukasz Majewski <lukma@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@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 think I got the point.

Even if we break "early" in SPL, we will not be able to recover as SPL
requires u-boot proper to take any action.

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

No. I do use 60s.

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

Yes. This is different. We increment bootcount when we do have
everything setup (just before we count down with "bootdelay").
WDT is setup earlier.

> Where is your
> watchdog now initialized in SPL? 

WDT is setup in board_init_f. Just after providing clocks.

> You should be okay, if the watchdog
> is enabled late in the SPL boot stage. Or am I missing something
> here?

No. I think that it would be safe to move the WDT and bootcount latter
in SPL.

> 
> Thanks,
> Stefan

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180226/e18fd2ca/attachment.sig>