| Message ID | 20180226122246.10595-5-lukma@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Provide SPL support for bootcount (in the case of using falcon boot mode) | expand |
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? Thanks, Stefan
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. Thanks for the idea. > > 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@denx.de
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. 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... Also the bootcount shall be incremented in the same function where we enable and initialize WDT. > > Thanks for the idea. > > > > > 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@denx.de 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@denx.de
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'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
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@denx.de
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(); + /* load/boot image from boot device */ board_init_r(NULL, 0); } @@ -214,7 +218,7 @@ void board_boot_order(u32 *spl_boot_list) env_load(); s = env_get("BOOT_FROM"); - if (s && strcmp(s, "ACTIVE") == 0) { + if (s && !bootcount_error() && strcmp(s, "ACTIVE") == 0) { spl_boot_list[0] = BOOT_DEVICE_MMC1; spl_boot_list[1] = spl_boot_device(); }
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(-)