[U-Boot] [PATCH v1 0/5] Provide SPL support for bootcount (in the case of using falcon boot mode)

[U-Boot] [PATCH v1 0/5] Provide SPL support for bootcount (in the case of using falcon boot mode)

[Lukasz Majewski]

This patch series provides support for controlling bootcount limits in SPL.
It also enables this feature on display5 board to present usage patterns.

This patch is probably eligible for -next u-boot release (after v2018.03).

This patch has been applied on top of u-boot/master:
SHA1: 85447f785ce8c0ab8e40850dc457a1fc833d224f



Lukasz Majewski (5):
  bootcount: spl: Enable bootcount support in SPL
  bootcount: Add function wrappers to handle bootcount increment and
    error checking
  bootcount: u-boot: Do not increment bootcount if already done in SPL
  bootcount: display5: spl: Extend SPL to support bootcount checking
  bootcount: display5: config: Enable boot count feature in the display5
    board

 board/liebherr/display5/spl.c |  6 +++++-
 common/autoboot.c             |  2 ++
 common/spl/Kconfig            |  9 +++++++++
 configs/display5_defconfig    |  4 ++++
 drivers/Makefile              |  1 +
 include/bootcount.h           | 25 +++++++++++++++++++++++++
 6 files changed, 46 insertions(+), 1 deletion(-)

-- 
2.11.0

[U-Boot] [PATCH v1 1/5] bootcount: spl: Enable bootcount support in SPL

[Lukasz Majewski]

New, SPL related config option - CONFIG_SPL_BOOTCOUNT_LIMIT has been
added to allow drivers/bootcount code re-usage in SPL.

This code is necessary to use and setup bootcount in SPL in the case of
falcon boot mode.

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---

 common/spl/Kconfig | 9 +++++++++
 drivers/Makefile   | 1 +
 2 files changed, 10 insertions(+)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index f58163ce53..b5aec981c8 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -54,6 +54,15 @@ config SPL_BOOTROM_SUPPORT
 	  BOOT_DEVICE_BOOTROM (or fall-through to the next boot device in the
 	  boot device list, if not implemented for a given board)
 
+config SPL_BOOTCOUNT_LIMIT
+	bool "Support bootcount in SPL"
+	depends on SPL_ENV_SUPPORT
+	help
+	  On some boards, which use 'falcon' mode, it is necessary to check
+	  and increment the number of boot attempts. Such boards do not
+	  use proper U-Boot for normal boot flow and hence needs those
+	  adjustments to be done in the SPL.
+
 config SPL_RAW_IMAGE_SUPPORT
 	bool "Support SPL loading and booting of RAW images"
 	default n if (ARCH_MX6 && (SPL_MMC_SUPPORT || SPL_SATA_SUPPORT))
diff --git a/drivers/Makefile b/drivers/Makefile
index e6062a5683..2a988360a0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_$(SPL_TPL_)TIMER) += timer/
 ifndef CONFIG_TPL_BUILD
 ifdef CONFIG_SPL_BUILD
 
+obj-$(CONFIG_SPL_BOOTCOUNT_LIMIT) += bootcount/
 obj-$(CONFIG_SPL_CPU_SUPPORT) += cpu/
 obj-$(CONFIG_SPL_CRYPTO_SUPPORT) += crypto/
 obj-$(CONFIG_SPL_GPIO_SUPPORT) += gpio/
-- 
2.11.0

[U-Boot] [PATCH v1 2/5] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

Those two functions can be used in either SPL or U-boot proper to provide
easy bootcount management.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---

 include/bootcount.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/bootcount.h b/include/bootcount.h
index 06fb4d3578..bdfec39e36 100644
--- a/include/bootcount.h
+++ b/include/bootcount.h
@@ -38,3 +38,28 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
 	return in_be32(addr);
 }
 #endif
+
+#ifdef CONFIG_SPL_BOOTCOUNT_LIMIT
+static inline bool bootcount_error(void)
+{
+	unsigned long bootcount = bootcount_load();
+	unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0);
+
+	if (bootlimit && (bootcount > bootlimit)) {
+		printf("Warning: Bootlimit (%lu) exceeded.\n", bootlimit);
+		return true;
+	}
+
+	return false;
+}
+
+static inline void bootcount_inc(void)
+{
+	unsigned long bootcount = bootcount_load();
+
+	bootcount_store(++bootcount);
+}
+#else
+static inline bool bootcount_error(void) { return false; }
+static inline void bootcount_inc(void) {}
+#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT */
-- 
2.11.0

[U-Boot] [PATCH v1 2/5] bootcount: Add function wrappers to handle bootcount increment and error checking

[Tom Rini]

On Mon, Feb 26, 2018 at 01:22:43PM +0100, Lukasz Majewski wrote:

> Those two functions can be used in either SPL or U-boot proper to provide
> easy bootcount management.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180226/787308ca/attachment.sig>

[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Lukasz Majewski]

If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount variable is
already incremented after each boot attempt.

For that reason we shall not increment it again in u-boot.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---

 common/autoboot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/autoboot.c b/common/autoboot.c
index 2eef7a04cc..87fca2ea92 100644
--- a/common/autoboot.c
+++ b/common/autoboot.c
@@ -298,7 +298,9 @@ const char *bootdelay_process(void)
 
 #ifdef CONFIG_BOOTCOUNT_LIMIT
 	bootcount = bootcount_load();
+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
 	bootcount++;
+#endif
 	bootcount_store(bootcount);
 	env_set_ulong("bootcount", bootcount);
 	bootlimit = env_get_ulong("bootlimit", 10, 0);
-- 
2.11.0

[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Tom Rini]

On Mon, Feb 26, 2018 at 01:22:44PM +0100, Lukasz Majewski wrote:

> If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount variable is
> already incremented after each boot attempt.
> 
> For that reason we shall not increment it again in u-boot.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> 
>  common/autoboot.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/autoboot.c b/common/autoboot.c
> index 2eef7a04cc..87fca2ea92 100644
> --- a/common/autoboot.c
> +++ b/common/autoboot.c
> @@ -298,7 +298,9 @@ const char *bootdelay_process(void)
>  
>  #ifdef CONFIG_BOOTCOUNT_LIMIT
>  	bootcount = bootcount_load();
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
>  	bootcount++;
> +#endif
>  	bootcount_store(bootcount);
>  	env_set_ulong("bootcount", bootcount);
>  	bootlimit = env_get_ulong("bootlimit", 10, 0);

Shouldn't we make the whole bit of code here depend on
!CONFIG_SPL_BOOTCOUNT_LIMIT ?  Or for that matter, re-work the first
patch to instead be:
bool SUPPORT_BOOTCOUNT "Support bootcount in some way"
choice "Bootcount location"
bool SPL_BOOTCOUNT_LIMIT "Count boots from POV of SPL"
bool BOOTCOUNT_LIMIT "Count boots from POV of full U-Boot"
endchoice

?  Looking back at my old series from 2014 to do this, I just had a big
warning saying you need to enable bootcount for SPL or for full U-Boot
so you would never set BOOTCOUNT_LIMIT if doing SPL bootcount instead.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180226/0442e0d1/attachment.sig>

[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Lukasz Majewski]

Hi Tom,

> On Mon, Feb 26, 2018 at 01:22:44PM +0100, Lukasz Majewski wrote:
> 
> > If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount
> > variable is already incremented after each boot attempt.
> > 
> > For that reason we shall not increment it again in u-boot.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > 
> >  common/autoboot.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/common/autoboot.c b/common/autoboot.c
> > index 2eef7a04cc..87fca2ea92 100644
> > --- a/common/autoboot.c
> > +++ b/common/autoboot.c
> > @@ -298,7 +298,9 @@ const char *bootdelay_process(void)
> >  
> >  #ifdef CONFIG_BOOTCOUNT_LIMIT
> >  	bootcount = bootcount_load();
> > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> >  	bootcount++;
> > +#endif
> >  	bootcount_store(bootcount);
> >  	env_set_ulong("bootcount", bootcount);
> >  	bootlimit = env_get_ulong("bootlimit", 10, 0);  
> 
> Shouldn't we make the whole bit of code here depend on
> !CONFIG_SPL_BOOTCOUNT_LIMIT ?  

Maybe I will try to present the scenario:

- We do use SPL to boot from either eMMC (if in falcon) or from SPI-NOR
  (if not in falcon)

- With bootcount:
	-- We may want to boot into eMMC directly, so we need to
	increment the bootcount (no matter where it is located).

	-- We intentionally (by setting other env variable) or not (by
	entering X resets) abort falcon mode boot from SPL and boot to
	u-boot (which is in spi-nor).
	In this case we do need all the bootcount limit functionality
	(i.e. altbootcmd) excluding increment bootcount (as it was
	done in SPL).
	
+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT is only required to achieve this
goal without making any more code change...

> Or for that matter, re-work the first
> patch to instead be:
> bool SUPPORT_BOOTCOUNT "Support bootcount in some way"
> choice "Bootcount location"
> bool SPL_BOOTCOUNT_LIMIT "Count boots from POV of SPL"
> bool BOOTCOUNT_LIMIT "Count boots from POV of full U-Boot"
> endchoice

But then SPL_BOOTCOUNT_LIMIT would select BOOTCOUNT_LIMIT to fulfil the
above scenario.

> 
> ?  Looking back at my old series from 2014 to do this, I just had a
> big warning saying you need to enable bootcount for SPL or for full
> U-Boot so you would never set BOOTCOUNT_LIMIT if doing SPL bootcount
> instead.

Such functionality can be achieved with SPL_BOOTCOUNT_LIMIT depending
on BOOTCOUNT_LIMIT.

If SPL_BOOTCOUNT_LIMIT selected, then "[PATCH v1 3/5] bootcount: u-boot:
Do not increment bootcount if already done in SPL" does its job.

> 




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/a68921cb/attachment.sig>

[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Tom Rini]

On Mon, Feb 26, 2018 at 03:53:18PM +0100, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Mon, Feb 26, 2018 at 01:22:44PM +0100, Lukasz Majewski wrote:
> > 
> > > If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount
> > > variable is already incremented after each boot attempt.
> > > 
> > > For that reason we shall not increment it again in u-boot.
> > > 
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > ---
> > > 
> > >  common/autoboot.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/common/autoboot.c b/common/autoboot.c
> > > index 2eef7a04cc..87fca2ea92 100644
> > > --- a/common/autoboot.c
> > > +++ b/common/autoboot.c
> > > @@ -298,7 +298,9 @@ const char *bootdelay_process(void)
> > >  
> > >  #ifdef CONFIG_BOOTCOUNT_LIMIT
> > >  	bootcount = bootcount_load();
> > > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > >  	bootcount++;
> > > +#endif
> > >  	bootcount_store(bootcount);
> > >  	env_set_ulong("bootcount", bootcount);
> > >  	bootlimit = env_get_ulong("bootlimit", 10, 0);  
> > 
> > Shouldn't we make the whole bit of code here depend on
> > !CONFIG_SPL_BOOTCOUNT_LIMIT ?  
> 
> Maybe I will try to present the scenario:
> 
> - We do use SPL to boot from either eMMC (if in falcon) or from SPI-NOR
>   (if not in falcon)
> 
> - With bootcount:
> 	-- We may want to boot into eMMC directly, so we need to
> 	increment the bootcount (no matter where it is located).
> 
> 	-- We intentionally (by setting other env variable) or not (by
> 	entering X resets) abort falcon mode boot from SPL and boot to
> 	u-boot (which is in spi-nor).
> 	In this case we do need all the bootcount limit functionality
> 	(i.e. altbootcmd) excluding increment bootcount (as it was
> 	done in SPL).
> 	
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT is only required to achieve this
> goal without making any more code change...

And you have one build that produces the binaries used in both cases,
OK.

> > Or for that matter, re-work the first
> > patch to instead be:
> > bool SUPPORT_BOOTCOUNT "Support bootcount in some way"
> > choice "Bootcount location"
> > bool SPL_BOOTCOUNT_LIMIT "Count boots from POV of SPL"
> > bool BOOTCOUNT_LIMIT "Count boots from POV of full U-Boot"
> > endchoice
> 
> But then SPL_BOOTCOUNT_LIMIT would select BOOTCOUNT_LIMIT to fulfil the
> above scenario.
> 
> > 
> > ?  Looking back at my old series from 2014 to do this, I just had a
> > big warning saying you need to enable bootcount for SPL or for full
> > U-Boot so you would never set BOOTCOUNT_LIMIT if doing SPL bootcount
> > instead.
> 
> Such functionality can be achieved with SPL_BOOTCOUNT_LIMIT depending
> on BOOTCOUNT_LIMIT.
> 
> If SPL_BOOTCOUNT_LIMIT selected, then "[PATCH v1 3/5] bootcount: u-boot:
> Do not increment bootcount if already done in SPL" does its job.

OK, I expect that once this code comes in, it might get complicated a
bit further by other use cases.  But I don't have a specific example I'm
going to implement currently, so I'm OK moving forward here and
expanding it as other cases come up.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180226/ccbcfa8a/attachment.sig>

[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Lukasz Majewski]

Hi Tom,

> On Mon, Feb 26, 2018 at 03:53:18PM +0100, Lukasz Majewski wrote:
> > Hi Tom,
> >   
> > > On Mon, Feb 26, 2018 at 01:22:44PM +0100, Lukasz Majewski wrote:
> > >   
> > > > If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount
> > > > variable is already incremented after each boot attempt.
> > > > 
> > > > For that reason we shall not increment it again in u-boot.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > > ---
> > > > 
> > > >  common/autoboot.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/common/autoboot.c b/common/autoboot.c
> > > > index 2eef7a04cc..87fca2ea92 100644
> > > > --- a/common/autoboot.c
> > > > +++ b/common/autoboot.c
> > > > @@ -298,7 +298,9 @@ const char *bootdelay_process(void)
> > > >  
> > > >  #ifdef CONFIG_BOOTCOUNT_LIMIT
> > > >  	bootcount = bootcount_load();
> > > > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > > >  	bootcount++;
> > > > +#endif
> > > >  	bootcount_store(bootcount);
> > > >  	env_set_ulong("bootcount", bootcount);
> > > >  	bootlimit = env_get_ulong("bootlimit", 10, 0);    
> > > 
> > > Shouldn't we make the whole bit of code here depend on
> > > !CONFIG_SPL_BOOTCOUNT_LIMIT ?    
> > 
> > Maybe I will try to present the scenario:
> > 
> > - We do use SPL to boot from either eMMC (if in falcon) or from
> > SPI-NOR (if not in falcon)
> > 
> > - With bootcount:
> > 	-- We may want to boot into eMMC directly, so we need to
> > 	increment the bootcount (no matter where it is located).
> > 
> > 	-- We intentionally (by setting other env variable) or not
> > (by entering X resets) abort falcon mode boot from SPL and boot to
> > 	u-boot (which is in spi-nor).
> > 	In this case we do need all the bootcount limit
> > functionality (i.e. altbootcmd) excluding increment bootcount (as
> > it was done in SPL).
> > 	
> > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT is only required to achieve this
> > goal without making any more code change...  
> 
> And you have one build that produces the binaries used in both cases,
> OK.

Yes, exactly. 

> 
> > > Or for that matter, re-work the first
> > > patch to instead be:
> > > bool SUPPORT_BOOTCOUNT "Support bootcount in some way"
> > > choice "Bootcount location"
> > > bool SPL_BOOTCOUNT_LIMIT "Count boots from POV of SPL"
> > > bool BOOTCOUNT_LIMIT "Count boots from POV of full U-Boot"
> > > endchoice  
> > 
> > But then SPL_BOOTCOUNT_LIMIT would select BOOTCOUNT_LIMIT to fulfil
> > the above scenario.
> >   
> > > 
> > > ?  Looking back at my old series from 2014 to do this, I just had
> > > a big warning saying you need to enable bootcount for SPL or for
> > > full U-Boot so you would never set BOOTCOUNT_LIMIT if doing SPL
> > > bootcount instead.  
> > 
> > Such functionality can be achieved with SPL_BOOTCOUNT_LIMIT
> > depending on BOOTCOUNT_LIMIT.
> > 
> > If SPL_BOOTCOUNT_LIMIT selected, then "[PATCH v1 3/5] bootcount:
> > u-boot: Do not increment bootcount if already done in SPL" does its
> > job.  
> 
> OK, I expect that once this code comes in, it might get complicated a
> bit further by other use cases.  But I don't have a specific example
> I'm going to implement currently, so I'm OK moving forward here and
> expanding it as other cases come up.  Thanks!
> 

No problem. 

As I've written in the cover letter - it would be great to
pull this code to -next branch, so it would be applied after we are done
with v2018.03


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/3f46787b/attachment.sig>

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

[Lukasz Majewski]

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();
+
 	/* 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();
 	}
-- 
2.11.0

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

[Stefan Roese]

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

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

[Lukasz Majewski]

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 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/4f5b9370/attachment.sig>

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

[Lukasz Majewski]

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 at 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 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/3f188068/attachment.sig>

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

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

[Lukasz Majewski]

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>

[U-Boot] [PATCH v1 5/5] bootcount: display5: config: Enable boot count feature in the display5 board

[Lukasz Majewski]

The boot count is enabled in both SPL and proper u-boot.
The SNVS_LPGPR register is used for storage.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---

 configs/display5_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configs/display5_defconfig b/configs/display5_defconfig
index 4d67700f4d..893794804c 100644
--- a/configs/display5_defconfig
+++ b/configs/display5_defconfig
@@ -16,6 +16,7 @@ CONFIG_OF_BOARD_SETUP=y
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=arch/arm/mach-imx/spl_sd.cfg,MX6Q"
 CONFIG_SUPPORT_RAW_INITRD=y
 CONFIG_SPL=y
+CONFIG_SPL_BOOTCOUNT_LIMIT=y
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
 CONFIG_SPL_DMA_SUPPORT=y
 CONFIG_SPL_ENV_SUPPORT=y
@@ -51,6 +52,9 @@ CONFIG_EFI_PARTITION=y
 # CONFIG_SPL_EFI_PARTITION is not set
 CONFIG_OF_CONTROL=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
+CONFIG_BOOTCOUNT_LIMIT=y
+CONFIG_SYS_BOOTCOUNT_SINGLEWORD=y
+CONFIG_SYS_BOOTCOUNT_ADDR=0x020CC068
 CONFIG_SPI_FLASH=y
 CONFIG_SPI_FLASH_BAR=y
 CONFIG_SPI_FLASH_SPANSION=y
-- 
2.11.0