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

[U-Boot] [PATCH v5 0/7] 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.
Moreover, the common code has been identified and reused in the
common/autoboot.c file.
It also enables this feature on display5 board to present usage patterns.

This patch has been applied on top of u-boot/master:
SHA1 : v2018.05-rc3

Test HW: Beagle Bone Black (am335x) , Display5 (imx6q)
Travis-Ci: https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971

Lukasz Majewski (7):
  bootcount: spl: Enable bootcount support in SPL
  bootcount: Add include guards into bootcount.h file
  bootcount: Add function wrappers to handle bootcount increment and
    error checking
  bootcount: Rewrite autoboot to use wrapper functions from bootcount.h
  bootcount: spl: Extend SPL to support bootcount incrementation
  bootcount: display5: spl: Extend DISPLAY5 board SPL to support
    bootcount checking
  bootcount: display5: config: Enable boot count feature in the display5
    board

 board/liebherr/display5/spl.c |  3 ++-
 common/autoboot.c             | 23 +++++---------------
 common/spl/Kconfig            |  9 ++++++++
 common/spl/spl.c              |  3 +++
 configs/display5_defconfig    |  4 ++++
 drivers/Makefile              |  1 +
 include/bootcount.h           | 50 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH v5 1/7] 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>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>


---

Changes in v5:
- None

Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- None

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

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 259f96607e..431710a93b 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 6846d181aa..061331eadd 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 v5 2/7] bootcount: Add include guards into bootcount.h file

[Lukasz Majewski]

This patch adds missing include guards for bootcount.h file.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>

---

Changes in v5:
- None

Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- New patch

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

diff --git a/include/bootcount.h b/include/bootcount.h
index 06fb4d3578..e3b3f7028e 100644
--- a/include/bootcount.h
+++ b/include/bootcount.h
@@ -4,6 +4,8 @@
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
+#ifndef _BOOTCOUNT_H__
+#define _BOOTCOUNT_H__
 
 #include <common.h>
 #include <asm/io.h>
@@ -38,3 +40,4 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
 	return in_be32(addr);
 }
 #endif
+#endif /* _BOOTCOUNT_H__ */
-- 
2.11.0

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

Those two functions can be used to provide easy bootcount management.

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

Reviewed-by: Tom Rini <trini@konsulko.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...

Changes in v4:
- Remove enum bootcount_context and replace it with checking gd_t->flags
  (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper,
   so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL context -
  Boards with TINY_PRINTF (in newest mainline) will build break as this function
  requires simple_itoa() from vsprintf.c (now not always build in SPL).

Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot proper and SPL

Changes in v2:
- None

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

diff --git a/include/bootcount.h b/include/bootcount.h
index e3b3f7028e..a886c98f44 100644
--- a/include/bootcount.h
+++ b/include/bootcount.h
@@ -11,6 +11,8 @@
 #include <asm/io.h>
 #include <asm/byteorder.h>
 
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT)
+
 #if !defined(CONFIG_SYS_BOOTCOUNT_LE) && !defined(CONFIG_SYS_BOOTCOUNT_BE)
 # if __BYTE_ORDER == __LITTLE_ENDIAN
 #  define CONFIG_SYS_BOOTCOUNT_LE
@@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
 	return in_be32(addr);
 }
 #endif
+
+DECLARE_GLOBAL_DATA_PTR;
+static inline int 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.", bootlimit);
+		if (!(gd->flags & GD_FLG_SPL_INIT))
+			printf(" Using altbootcmd.");
+		printf("\n");
+
+		return 1;
+	}
+
+	return 0;
+}
+
+static inline void bootcount_inc(void)
+{
+	unsigned long bootcount = bootcount_load();
+
+	if (gd->flags & GD_FLG_SPL_INIT) {
+		bootcount_store(++bootcount);
+		return;
+	}
+
+#ifndef CONFIG_SPL_BUILD
+	/* Only increment bootcount when no bootcount support in SPL */
+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
+	bootcount_store(++bootcount);
+#endif
+	env_set_ulong("bootcount", bootcount);
+#endif /* !CONFIG_SPL_BUILD */
+}
+
+#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT)
+void bootcount_store(ulong a) {};
+ulong bootcount_load(void) { return 0; }
+#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */
+#else
+static inline int bootcount_error(void) { return 0; }
+static inline void bootcount_inc(void) {}
+#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
 #endif /* _BOOTCOUNT_H__ */
-- 
2.11.0

[U-Boot] [PATCH v5 4/7] bootcount: Rewrite autoboot to use wrapper functions from bootcount.h

[Lukasz Majewski]

The code has been refactored to use common wrappers from bootcount.h
header.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>

---

Changes in v5:
- None

Changes in v4:
- Use global data pointer (gd_t *) instead of bootcount specific enum

Changes in v3:
- New patch

Changes in v2: None

 common/autoboot.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/common/autoboot.c b/common/autoboot.c
index 2eef7a04cc..a0f7822c9e 100644
--- a/common/autoboot.c
+++ b/common/autoboot.c
@@ -14,6 +14,7 @@
 #include <menu.h>
 #include <post.h>
 #include <u-boot/sha256.h>
+#include <bootcount.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -291,18 +292,8 @@ const char *bootdelay_process(void)
 {
 	char *s;
 	int bootdelay;
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	unsigned long bootcount = 0;
-	unsigned long bootlimit = 0;
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	bootcount = bootcount_load();
-	bootcount++;
-	bootcount_store(bootcount);
-	env_set_ulong("bootcount", bootcount);
-	bootlimit = env_get_ulong("bootlimit", 10, 0);
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
+
+	bootcount_inc();
 
 	s = env_get("bootdelay");
 	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
@@ -324,13 +315,9 @@ const char *bootdelay_process(void)
 		s = env_get("failbootcmd");
 	} else
 #endif /* CONFIG_POST */
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	if (bootlimit && (bootcount > bootlimit)) {
-		printf("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n",
-		       (unsigned)bootlimit);
+	if (bootcount_error())
 		s = env_get("altbootcmd");
-	} else
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
+	else
 		s = env_get("bootcmd");
 
 	process_fdt_options(gd->fdt_blob);
-- 
2.11.0

[U-Boot] [PATCH v5 5/7] bootcount: spl: Extend SPL to support bootcount incrementation

[Lukasz Majewski]

This patch adds support for incrementation of the bootcount in SPL.
Such feature is necessary when we do want to use this feature with
'falcon' boot mode (which loads OS directly in SPL).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>

---

Changes in v5:
- None

Changes in v4:
- Use gd_t global data pointer instead of bootcount specific enum

Changes in v3:
- Remove not needed #ifdefs
- Add enum bootcount_context parameter to bootcount_inc() function

Changes in v2:
- New patch - as suggested by Stefan Roese - bootcount_inc() is called
  in common SPL code (./common/spl/spl.c), so other boards can also
  reuse it without modification

 common/spl/spl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 794dbd0312..6eb50f3534 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -20,6 +20,7 @@
 #include <dm/root.h>
 #include <linux/compiler.h>
 #include <fdt_support.h>
+#include <bootcount.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -417,6 +418,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	spl_board_init();
 #endif
 
+	bootcount_inc();
+
 	memset(&spl_image, '\0', sizeof(spl_image));
 #ifdef CONFIG_SYS_SPL_ARGS_ADDR
 	spl_image.arg = (void *)CONFIG_SYS_SPL_ARGS_ADDR;
-- 
2.11.0

[U-Boot] [PATCH v5 6/7] bootcount: display5: spl: Extend DISPLAY5 board SPL to support bootcount checking

[Lukasz Majewski]

This patch is necessary for providing basic bootcount checking in the case
of using "falcon" boot mode in that board.

It forces u-boot proper boot, when we exceed the number of errors.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>

---

Changes in v5:
- None

Changes in v4:
- Use global data pointer (gd) instead of bootcount specific enum (SPL)

Changes in v3:
- The bootcount_error now accepts enum bootcount_error input parameter

Changes in v2:
- Remove bootcount_init() from SPL specific board code

 board/liebherr/display5/spl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c
index 437963e225..7712e5bc3f 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;
@@ -214,7 +215,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 v5 7/7] 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.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>

---

Changes in v5:
- None

Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- None

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

diff --git a/configs/display5_defconfig b/configs/display5_defconfig
index e52f4e00af..db8212ca7c 100644
--- a/configs/display5_defconfig
+++ b/configs/display5_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_LOAD_FIT=y
 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_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
@@ -53,6 +54,9 @@ CONFIG_EFI_PARTITION=y
 CONFIG_OF_CONTROL=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_FSL_ESDHC=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

[U-Boot] [PATCH v5 1/7] bootcount: spl: Enable bootcount support in SPL

[Alex Kiernan]

On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:

> 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>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>


Reviewed-by: Alex Kiernan <alex.kiernan@gmail.com>


> ---

> Changes in v5:
> - None

> Changes in v4:
> - None

> Changes in v3:
> - None

> Changes in v2:
> - None

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

> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 259f96607e..431710a93b 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 6846d181aa..061331eadd 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



-- 
Alex Kiernan

[U-Boot] [PATCH v5 2/7] bootcount: Add include guards into bootcount.h file

[Alex Kiernan]

On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:

> This patch adds missing include guards for bootcount.h file.

> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>


Reviewed-by: Alex Kiernan <alex.kiernan@gmail.com>

> ---

> Changes in v5:
> - None

> Changes in v4:
> - None

> Changes in v3:
> - None

> Changes in v2:
> - New patch

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

> diff --git a/include/bootcount.h b/include/bootcount.h
> index 06fb4d3578..e3b3f7028e 100644
> --- a/include/bootcount.h
> +++ b/include/bootcount.h
> @@ -4,6 +4,8 @@
>    *
>    * SPDX-License-Identifier:    GPL-2.0+
>    */
> +#ifndef _BOOTCOUNT_H__
> +#define _BOOTCOUNT_H__

>   #include <common.h>
>   #include <asm/io.h>
> @@ -38,3 +40,4 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
>          return in_be32(addr);
>   }
>   #endif
> +#endif /* _BOOTCOUNT_H__ */
> --
> 2.11.0



-- 
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Alex Kiernan]

On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:

> Those two functions can be used to provide easy bootcount management.

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

> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---

> Changes in v5:
> - Provide parenthesis for #if defined(FOO) && ...

> Changes in v4:
> - Remove enum bootcount_context and replace it with checking gd_t->flags
>    (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper,
>     so can be used as indication if we are in u-boot or SPL).
> - Do not call bootcount_store() twice when it is not needed.
> - Call env_set_ulong("bootcount", bootcount); only in NON SPL context -
>    Boards with TINY_PRINTF (in newest mainline) will build break as this
function
>    requires simple_itoa() from vsprintf.c (now not always build in SPL).

> Changes in v3:
> - Unify those functions to also work with common/autoboot.c code
> - Add enum bootcount_context to distinguish between u-boot proper and SPL

> Changes in v2:
> - None

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

> diff --git a/include/bootcount.h b/include/bootcount.h
> index e3b3f7028e..a886c98f44 100644
> --- a/include/bootcount.h
> +++ b/include/bootcount.h
> @@ -11,6 +11,8 @@
>   #include <asm/io.h>
>   #include <asm/byteorder.h>

> +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
> +
>   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
>   # if __BYTE_ORDER == __LITTLE_ENDIAN
>   #  define CONFIG_SYS_BOOTCOUNT_LE
> @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
>          return in_be32(addr);
>   }
>   #endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +static inline int 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.", bootlimit);
> +               if (!(gd->flags & GD_FLG_SPL_INIT))
> +                       printf(" Using altbootcmd.");
> +               printf("\n");
> +
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +static inline void bootcount_inc(void)
> +{
> +       unsigned long bootcount = bootcount_load();
> +
> +       if (gd->flags & GD_FLG_SPL_INIT) {
> +               bootcount_store(++bootcount);
> +               return;
> +       }
> +
> +#ifndef CONFIG_SPL_BUILD
> +       /* Only increment bootcount when no bootcount support in SPL */
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> +       bootcount_store(++bootcount);
> +#endif
> +       env_set_ulong("bootcount", bootcount);
> +#endif /* !CONFIG_SPL_BUILD */
> +}
> +

I'm kinda confused by this code... isn't this equivalent.?

   static inline void bootcount_inc(void)
   {
          unsigned long bootcount = bootcount_load();

          bootcount_store(++bootcount);
   #ifndef CONFIG_SPL_BUILD
          env_set_ulong("bootcount", bootcount);
   #endif /* !CONFIG_SPL_BUILD */
   }

Also I suspect bootcount_store() will fail at link time on boards where the
bootcount is stored in ext4?

> +#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT)
> +void bootcount_store(ulong a) {};
> +ulong bootcount_load(void) { return 0; }
> +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */
> +#else
> +static inline int bootcount_error(void) { return 0; }
> +static inline void bootcount_inc(void) {}
> +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
>   #endif /* _BOOTCOUNT_H__ */
> --
> 2.11.0



-- 
Alex Kiernan

[U-Boot] [PATCH v5 4/7] bootcount: Rewrite autoboot to use wrapper functions from bootcount.h

[Alex Kiernan]

On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:

> The code has been refactored to use common wrappers from bootcount.h
> header.

> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Reviewed-by:  Alex Kiernan <alex.kiernan@gmail.com>


> ---

> Changes in v5:
> - None

> Changes in v4:
> - Use global data pointer (gd_t *) instead of bootcount specific enum

> Changes in v3:
> - New patch

> Changes in v2: None

>   common/autoboot.c | 23 +++++------------------
>   1 file changed, 5 insertions(+), 18 deletions(-)

> diff --git a/common/autoboot.c b/common/autoboot.c
> index 2eef7a04cc..a0f7822c9e 100644
> --- a/common/autoboot.c
> +++ b/common/autoboot.c
> @@ -14,6 +14,7 @@
>   #include <menu.h>
>   #include <post.h>
>   #include <u-boot/sha256.h>
> +#include <bootcount.h>

>   DECLARE_GLOBAL_DATA_PTR;

> @@ -291,18 +292,8 @@ const char *bootdelay_process(void)
>   {
>          char *s;
>          int bootdelay;
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> -       unsigned long bootcount = 0;
> -       unsigned long bootlimit = 0;
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> -
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> -       bootcount = bootcount_load();
> -       bootcount++;
> -       bootcount_store(bootcount);
> -       env_set_ulong("bootcount", bootcount);
> -       bootlimit = env_get_ulong("bootlimit", 10, 0);
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> +
> +       bootcount_inc();

>          s = env_get("bootdelay");
>          bootdelay = s ? (int)simple_strtol(s, NULL, 10) :
CONFIG_BOOTDELAY;
> @@ -324,13 +315,9 @@ const char *bootdelay_process(void)
>                  s = env_get("failbootcmd");
>          } else
>   #endif /* CONFIG_POST */
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> -       if (bootlimit && (bootcount > bootlimit)) {
> -               printf("Warning: Bootlimit (%u) exceeded. Using
altbootcmd.\n",
> -                      (unsigned)bootlimit);
> +       if (bootcount_error())
>                  s = env_get("altbootcmd");
> -       } else
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> +       else
>                  s = env_get("bootcmd");

>          process_fdt_options(gd->fdt_blob);
> --
> 2.11.0



-- 
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Alex Kiernan]

On Tue, May 8, 2018 at 6:15 AM Alex Kiernan <alex.kiernan@gmail.com> wrote:


> On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:

> > Those two functions can be used to provide easy bootcount management.

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

> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > ---

> > Changes in v5:
> > - Provide parenthesis for #if defined(FOO) && ...

> > Changes in v4:
> > - Remove enum bootcount_context and replace it with checking gd_t->flags
> >    (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot
proper,
> >     so can be used as indication if we are in u-boot or SPL).
> > - Do not call bootcount_store() twice when it is not needed.
> > - Call env_set_ulong("bootcount", bootcount); only in NON SPL context -
> >    Boards with TINY_PRINTF (in newest mainline) will build break as this
> function
> >    requires simple_itoa() from vsprintf.c (now not always build in SPL).

> > Changes in v3:
> > - Unify those functions to also work with common/autoboot.c code
> > - Add enum bootcount_context to distinguish between u-boot proper and
SPL

> > Changes in v2:
> > - None

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

> > diff --git a/include/bootcount.h b/include/bootcount.h
> > index e3b3f7028e..a886c98f44 100644
> > --- a/include/bootcount.h
> > +++ b/include/bootcount.h
> > @@ -11,6 +11,8 @@
> >   #include <asm/io.h>
> >   #include <asm/byteorder.h>

> > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> defined(CONFIG_BOOTCOUNT_LIMIT)
> > +
> >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
> !defined(CONFIG_SYS_BOOTCOUNT_BE)
> >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
> *addr)
> >          return in_be32(addr);
> >   }
> >   #endif
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +static inline int 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.", bootlimit);
> > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > +                       printf(" Using altbootcmd.");
> > +               printf("\n");
> > +
> > +               return 1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static inline void bootcount_inc(void)
> > +{
> > +       unsigned long bootcount = bootcount_load();
> > +
> > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > +               bootcount_store(++bootcount);
> > +               return;
> > +       }
> > +
> > +#ifndef CONFIG_SPL_BUILD
> > +       /* Only increment bootcount when no bootcount support in SPL */
> > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > +       bootcount_store(++bootcount);
> > +#endif
> > +       env_set_ulong("bootcount", bootcount);
> > +#endif /* !CONFIG_SPL_BUILD */
> > +}
> > +

> I'm kinda confused by this code... isn't this equivalent.?

>      static inline void bootcount_inc(void)
>      {
>             unsigned long bootcount = bootcount_load();

>             bootcount_store(++bootcount);
>      #ifndef CONFIG_SPL_BUILD
>             env_set_ulong("bootcount", bootcount);
>      #endif /* !CONFIG_SPL_BUILD */
>      }

I should've included my reasoning as I've got to be missing something... if
GD_FLG_SPL_INIT is always set when we get here in SPL, then it's equivalent
to the compile time guard. Which I think says I don't understand the flow
to how we get here, otherwise we wouldn't need the runtime guard.

> > +#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT)
> > +void bootcount_store(ulong a) {};
> > +ulong bootcount_load(void) { return 0; }
> > +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */
> > +#else
> > +static inline int bootcount_error(void) { return 0; }
> > +static inline void bootcount_inc(void) {}
> > +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
> >   #endif /* _BOOTCOUNT_H__ */
> > --
> > 2.11.0



> --
> Alex Kiernan



--
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Stefan Roese]

On 08.05.2018 08:58, Alex Kiernan wrote:

<snip>

>>> +static inline void bootcount_inc(void)
>>> +{
>>> +       unsigned long bootcount = bootcount_load();
>>> +
>>> +       if (gd->flags & GD_FLG_SPL_INIT) {
>>> +               bootcount_store(++bootcount);
>>> +               return;
>>> +       }
>>> +
>>> +#ifndef CONFIG_SPL_BUILD
>>> +       /* Only increment bootcount when no bootcount support in SPL */
>>> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
>>> +       bootcount_store(++bootcount);
>>> +#endif
>>> +       env_set_ulong("bootcount", bootcount);
>>> +#endif /* !CONFIG_SPL_BUILD */
>>> +}
>>> +
> 
>> I'm kinda confused by this code... isn't this equivalent.?
> 
>>       static inline void bootcount_inc(void)
>>       {
>>              unsigned long bootcount = bootcount_load();
> 
>>              bootcount_store(++bootcount);
>>       #ifndef CONFIG_SPL_BUILD
>>              env_set_ulong("bootcount", bootcount);
>>       #endif /* !CONFIG_SPL_BUILD */
>>       }
> 
> I should've included my reasoning as I've got to be missing something... if
> GD_FLG_SPL_INIT is always set when we get here in SPL, then it's equivalent
> to the compile time guard. Which I think says I don't understand the flow
> to how we get here, otherwise we wouldn't need the runtime guard.

When using with SPL and bootcounter support, this code will get
called twice, first from the SPL, where the counter will get
incremented. And second from main U-Boot, where we need to make
sure, that the counter does not get incremented again, if SPL
has already done so.

With your patch version, the bootcounter would get incremented
twice in this case.

Thanks,
Stefan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

Hi Alex, Stefan,

> On 08.05.2018 08:58, Alex Kiernan wrote:
> 
> <snip>
> 
> >>> +static inline void bootcount_inc(void)
> >>> +{
> >>> +       unsigned long bootcount = bootcount_load();
> >>> +
> >>> +       if (gd->flags & GD_FLG_SPL_INIT) {
> >>> +               bootcount_store(++bootcount);
> >>> +               return;
> >>> +       }
> >>> +
> >>> +#ifndef CONFIG_SPL_BUILD
> >>> +       /* Only increment bootcount when no bootcount support in
> >>> SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> >>> +       bootcount_store(++bootcount);
> >>> +#endif
> >>> +       env_set_ulong("bootcount", bootcount);
> >>> +#endif /* !CONFIG_SPL_BUILD */
> >>> +}
> >>> +  
> >   
> >> I'm kinda confused by this code... isn't this equivalent.?  
> >   
> >>       static inline void bootcount_inc(void)
> >>       {
> >>              unsigned long bootcount = bootcount_load();  
> >   
> >>              bootcount_store(++bootcount);
> >>       #ifndef CONFIG_SPL_BUILD
> >>              env_set_ulong("bootcount", bootcount);
> >>       #endif /* !CONFIG_SPL_BUILD */
> >>       }  
> > 
> > I should've included my reasoning as I've got to be missing
> > something... if GD_FLG_SPL_INIT is always set when we get here in
> > SPL, then it's equivalent to the compile time guard. Which I think
> > says I don't understand the flow to how we get here, otherwise we
> > wouldn't need the runtime guard.  
> 
> When using with SPL and bootcounter support, this code will get
> called twice, first from the SPL, where the counter will get
> incremented. And second from main U-Boot, where we need to make
> sure, that the counter does not get incremented again, if SPL
> has already done so.

+1

> 
> With your patch version, the bootcounter would get incremented
> twice in this case.
> 
> 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/20180508/f0450882/attachment.sig>

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

On Tue, 08 May 2018 05:15:13 +0000
Alex Kiernan <alex.kiernan@gmail.com> wrote:

> On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:
> 
> > Those two functions can be used to provide easy bootcount
> > management.  
> 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > ---  
> 
> > Changes in v5:
> > - Provide parenthesis for #if defined(FOO) && ...  
> 
> > Changes in v4:
> > - Remove enum bootcount_context and replace it with checking
> > gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared
> > in u-boot proper, so can be used as indication if we are in u-boot
> > or SPL).
> > - Do not call bootcount_store() twice when it is not needed.
> > - Call env_set_ulong("bootcount", bootcount); only in NON SPL
> > context - Boards with TINY_PRINTF (in newest mainline) will build
> > break as this  
> function
> >    requires simple_itoa() from vsprintf.c (now not always build in
> > SPL).  
> 
> > Changes in v3:
> > - Unify those functions to also work with common/autoboot.c code
> > - Add enum bootcount_context to distinguish between u-boot proper
> > and SPL  
> 
> > Changes in v2:
> > - None  
> 
> >   include/bootcount.h | 47
> > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47
> > insertions(+)  
> 
> > diff --git a/include/bootcount.h b/include/bootcount.h
> > index e3b3f7028e..a886c98f44 100644
> > --- a/include/bootcount.h
> > +++ b/include/bootcount.h
> > @@ -11,6 +11,8 @@
> >   #include <asm/io.h>
> >   #include <asm/byteorder.h>  
> 
> > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||  
> defined(CONFIG_BOOTCOUNT_LIMIT)
> > +
> >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&  
> !defined(CONFIG_SYS_BOOTCOUNT_BE)
> >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile
> > u32  
> *addr)
> >          return in_be32(addr);
> >   }
> >   #endif
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +static inline int 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.",
> > bootlimit);
> > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > +                       printf(" Using altbootcmd.");
> > +               printf("\n");
> > +
> > +               return 1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static inline void bootcount_inc(void)
> > +{
> > +       unsigned long bootcount = bootcount_load();
> > +
> > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > +               bootcount_store(++bootcount);
> > +               return;
> > +       }
> > +
> > +#ifndef CONFIG_SPL_BUILD
> > +       /* Only increment bootcount when no bootcount support in
> > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > +       bootcount_store(++bootcount);
> > +#endif
> > +       env_set_ulong("bootcount", bootcount);
> > +#endif /* !CONFIG_SPL_BUILD */
> > +}
> > +  
> 
> I'm kinda confused by this code... isn't this equivalent.?
> 
>    static inline void bootcount_inc(void)
>    {
>           unsigned long bootcount = bootcount_load();
> 
>           bootcount_store(++bootcount);
>    #ifndef CONFIG_SPL_BUILD
>           env_set_ulong("bootcount", bootcount);
>    #endif /* !CONFIG_SPL_BUILD */
>    }
> 
> Also I suspect bootcount_store() will fail at link time on boards
> where the bootcount is stored in ext4?

I've run this patch set several times with travis-CI. No errors were
present (the travis-ci link is in the cover letter).

Maybe there are some out of tree boards, which use the bootcount in
some "odd" way....

> 
> > +#if defined(CONFIG_SPL_BUILD)
> > && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT) +void bootcount_store(ulong
> > a) {}; +ulong bootcount_load(void) { return 0; }
> > +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */
> > +#else
> > +static inline int bootcount_error(void) { return 0; }
> > +static inline void bootcount_inc(void) {}
> > +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
> >   #endif /* _BOOTCOUNT_H__ */
> > --
> > 2.11.0  
> 
> 
> 




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/20180508/1ac80274/attachment.sig>

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Alex Kiernan]

On Tue, May 8, 2018 at 8:11 AM Stefan Roese <sr@denx.de> wrote:

> On 08.05.2018 08:58, Alex Kiernan wrote:

> <snip>

> >>> +static inline void bootcount_inc(void)
> >>> +{
> >>> +       unsigned long bootcount = bootcount_load();
> >>> +
> >>> +       if (gd->flags & GD_FLG_SPL_INIT) {
> >>> +               bootcount_store(++bootcount);
> >>> +               return;
> >>> +       }
> >>> +
> >>> +#ifndef CONFIG_SPL_BUILD
> >>> +       /* Only increment bootcount when no bootcount support in SPL
*/
> >>> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> >>> +       bootcount_store(++bootcount);
> >>> +#endif
> >>> +       env_set_ulong("bootcount", bootcount);
> >>> +#endif /* !CONFIG_SPL_BUILD */
> >>> +}
> >>> +
> >
> >> I'm kinda confused by this code... isn't this equivalent.?
> >
> >>       static inline void bootcount_inc(void)
> >>       {
> >>              unsigned long bootcount = bootcount_load();
> >
> >>              bootcount_store(++bootcount);
> >>       #ifndef CONFIG_SPL_BUILD
> >>              env_set_ulong("bootcount", bootcount);
> >>       #endif /* !CONFIG_SPL_BUILD */
> >>       }
> >
> > I should've included my reasoning as I've got to be missing
something... if
> > GD_FLG_SPL_INIT is always set when we get here in SPL, then it's
equivalent
> > to the compile time guard. Which I think says I don't understand the
flow
> > to how we get here, otherwise we wouldn't need the runtime guard.

> When using with SPL and bootcounter support, this code will get
> called twice, first from the SPL, where the counter will get
> incremented. And second from main U-Boot, where we need to make
> sure, that the counter does not get incremented again, if SPL
> has already done so.

> With your patch version, the bootcounter would get incremented
> twice in this case.


Ahh... thank you. That was the important piece!

-- 
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Alex Kiernan]

On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski <lukma@denx.de> wrote:

> On Tue, 08 May 2018 05:15:13 +0000
> Alex Kiernan <alex.kiernan@gmail.com> wrote:

> > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > > Those two functions can be used to provide easy bootcount
> > > management.
> >
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >
> > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > ---
> >
> > > Changes in v5:
> > > - Provide parenthesis for #if defined(FOO) && ...
> >
> > > Changes in v4:
> > > - Remove enum bootcount_context and replace it with checking
> > > gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared
> > > in u-boot proper, so can be used as indication if we are in u-boot
> > > or SPL).
> > > - Do not call bootcount_store() twice when it is not needed.
> > > - Call env_set_ulong("bootcount", bootcount); only in NON SPL
> > > context - Boards with TINY_PRINTF (in newest mainline) will build
> > > break as this
> > function
> > >    requires simple_itoa() from vsprintf.c (now not always build in
> > > SPL).
> >
> > > Changes in v3:
> > > - Unify those functions to also work with common/autoboot.c code
> > > - Add enum bootcount_context to distinguish between u-boot proper
> > > and SPL
> >
> > > Changes in v2:
> > > - None
> >
> > >   include/bootcount.h | 47
> > > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47
> > > insertions(+)
> >
> > > diff --git a/include/bootcount.h b/include/bootcount.h
> > > index e3b3f7028e..a886c98f44 100644
> > > --- a/include/bootcount.h
> > > +++ b/include/bootcount.h
> > > @@ -11,6 +11,8 @@
> > >   #include <asm/io.h>
> > >   #include <asm/byteorder.h>
> >
> > > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> > defined(CONFIG_BOOTCOUNT_LIMIT)
> > > +
> > >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
> > !defined(CONFIG_SYS_BOOTCOUNT_BE)
> > >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> > >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > > @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile
> > > u32
> > *addr)
> > >          return in_be32(addr);
> > >   }
> > >   #endif
> > > +
> > > +DECLARE_GLOBAL_DATA_PTR;
> > > +static inline int 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.",
> > > bootlimit);
> > > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > > +                       printf(" Using altbootcmd.");
> > > +               printf("\n");
> > > +
> > > +               return 1;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static inline void bootcount_inc(void)
> > > +{
> > > +       unsigned long bootcount = bootcount_load();
> > > +
> > > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > > +               bootcount_store(++bootcount);
> > > +               return;
> > > +       }
> > > +
> > > +#ifndef CONFIG_SPL_BUILD
> > > +       /* Only increment bootcount when no bootcount support in
> > > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > > +       bootcount_store(++bootcount);
> > > +#endif
> > > +       env_set_ulong("bootcount", bootcount);
> > > +#endif /* !CONFIG_SPL_BUILD */
> > > +}
> > > +
> >
> > I'm kinda confused by this code... isn't this equivalent.?
> >
> >    static inline void bootcount_inc(void)
> >    {
> >           unsigned long bootcount = bootcount_load();
> >
> >           bootcount_store(++bootcount);
> >    #ifndef CONFIG_SPL_BUILD
> >           env_set_ulong("bootcount", bootcount);
> >    #endif /* !CONFIG_SPL_BUILD */
> >    }
> >
> > Also I suspect bootcount_store() will fail at link time on boards
> > where the bootcount is stored in ext4?

> I've run this patch set several times with travis-CI. No errors were
> present (the travis-ci link is in the cover letter).

> Maybe there are some out of tree boards, which use the bootcount in
> some "odd" way....


I'm only really aware of the ext4 stuff from when I picked through it to
consolidate the bootcount into Kconfig. I don't think there would be any
Travis failures for this case as you need to have bootcount in SPL which
you can't have before this series. If I try building a board like this
(choose ext4 for the bootcount) and it fails to link:

drivers/built-in.o: In function `bootcount_store':
u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to
`fs_set_blk_dev'
u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to
`fs_write'
drivers/built-in.o: In function `bootcount_load':
u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to
`fs_set_blk_dev'
u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to
`fs_read'

But having just played around with Kconfig a bit for this case, I'm not
sure there's anything that's actually any better than a link time error;
anything we did in Kconfig would just end up modelling out that link error.

-- 
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

Hi Alex,

> On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski <lukma@denx.de> wrote:
> 
> > On Tue, 08 May 2018 05:15:13 +0000
> > Alex Kiernan <alex.kiernan@gmail.com> wrote:  
> 
> > > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de>
> > > wrote: 
> > > > Those two functions can be used to provide easy bootcount
> > > > management.  
> > >  
> > > > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> > >  
> > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > > ---  
> > >  
> > > > Changes in v5:
> > > > - Provide parenthesis for #if defined(FOO) && ...  
> > >  
> > > > Changes in v4:
> > > > - Remove enum bootcount_context and replace it with checking
> > > > gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is
> > > > cleared in u-boot proper, so can be used as indication if we
> > > > are in u-boot or SPL).
> > > > - Do not call bootcount_store() twice when it is not needed.
> > > > - Call env_set_ulong("bootcount", bootcount); only in NON SPL
> > > > context - Boards with TINY_PRINTF (in newest mainline) will
> > > > build break as this  
> > > function  
> > > >    requires simple_itoa() from vsprintf.c (now not always build
> > > > in SPL).  
> > >  
> > > > Changes in v3:
> > > > - Unify those functions to also work with common/autoboot.c code
> > > > - Add enum bootcount_context to distinguish between u-boot
> > > > proper and SPL  
> > >  
> > > > Changes in v2:
> > > > - None  
> > >  
> > > >   include/bootcount.h | 47
> > > > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > > 47 insertions(+)  
> > >  
> > > > diff --git a/include/bootcount.h b/include/bootcount.h
> > > > index e3b3f7028e..a886c98f44 100644
> > > > --- a/include/bootcount.h
> > > > +++ b/include/bootcount.h
> > > > @@ -11,6 +11,8 @@
> > > >   #include <asm/io.h>
> > > >   #include <asm/byteorder.h>  
> > >  
> > > > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||  
> > > defined(CONFIG_BOOTCOUNT_LIMIT)  
> > > > +
> > > >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&  
> > > !defined(CONFIG_SYS_BOOTCOUNT_BE)  
> > > >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> > > >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > > > @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile
> > > > u32  
> > > *addr)  
> > > >          return in_be32(addr);
> > > >   }
> > > >   #endif
> > > > +
> > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > +static inline int 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.",
> > > > bootlimit);
> > > > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > > > +                       printf(" Using altbootcmd.");
> > > > +               printf("\n");
> > > > +
> > > > +               return 1;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static inline void bootcount_inc(void)
> > > > +{
> > > > +       unsigned long bootcount = bootcount_load();
> > > > +
> > > > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > > > +               bootcount_store(++bootcount);
> > > > +               return;
> > > > +       }
> > > > +
> > > > +#ifndef CONFIG_SPL_BUILD
> > > > +       /* Only increment bootcount when no bootcount support in
> > > > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > > > +       bootcount_store(++bootcount);
> > > > +#endif
> > > > +       env_set_ulong("bootcount", bootcount);
> > > > +#endif /* !CONFIG_SPL_BUILD */
> > > > +}
> > > > +  
> > >
> > > I'm kinda confused by this code... isn't this equivalent.?
> > >
> > >    static inline void bootcount_inc(void)
> > >    {
> > >           unsigned long bootcount = bootcount_load();
> > >
> > >           bootcount_store(++bootcount);
> > >    #ifndef CONFIG_SPL_BUILD
> > >           env_set_ulong("bootcount", bootcount);
> > >    #endif /* !CONFIG_SPL_BUILD */
> > >    }
> > >
> > > Also I suspect bootcount_store() will fail at link time on boards
> > > where the bootcount is stored in ext4?  
> 
> > I've run this patch set several times with travis-CI. No errors were
> > present (the travis-ci link is in the cover letter).  
> 
> > Maybe there are some out of tree boards, which use the bootcount in
> > some "odd" way....  
> 
> 
> I'm only really aware of the ext4 stuff from when I picked through it
> to consolidate the bootcount into Kconfig. I don't think there would
> be any Travis failures for this case as you need to have bootcount in
> SPL which you can't have before this series. 

Could you be more specific here?

This series adds support for bootcount in SPL. The travis-CI tests
which I've performed were correct for the all boards which use it:

https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971

The bootcount_inc() is added in generic SPL code - this seems to work
on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT will
just return from it.

However, I don't know how the code will behave if one would like to add
ext4 support to it (including ext4 support to SPL).

I suppose that those boards will not enable SPL BOOTCOUNT in SPL and
use the code as it is now - just in u-boot.

This patch series was designed with one main use case in mind - the
"falcon" boot of Linux from SPL with bootcount support.

> If I try building a
> board like this (choose ext4 for the bootcount) and it fails to link:
> 
> drivers/built-in.o: In function `bootcount_store':
> u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to
> `fs_set_blk_dev'
> u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to
> `fs_write'
> drivers/built-in.o: In function `bootcount_load':
> u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to
> `fs_set_blk_dev'
> u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to
> `fs_read'
> 
> But having just played around with Kconfig a bit for this case, I'm
> not sure there's anything that's actually any better than a link time
> error; anything we did in Kconfig would just end up modelling out
> that link error.
> 

Could you share which particular board this breaks? I've tested it on
Beagle Bone Black (and display5).


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/20180508/23d18e25/attachment.sig>

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Alex Kiernan]

On Tue, May 8, 2018 at 10:21 AM Lukasz Majewski <lukma@denx.de> wrote:

> Hi Alex,

> > On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > > On Tue, 08 May 2018 05:15:13 +0000
> > > Alex Kiernan <alex.kiernan@gmail.com> wrote:
> >
> > > > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de>
> > > > wrote:
> > > > > Those two functions can be used to provide easy bootcount
> > > > > management.
> > > >
> > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > >
> > > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > > > ---
> > > >
> > > > > Changes in v5:
> > > > > - Provide parenthesis for #if defined(FOO) && ...
> > > >
> > > > > Changes in v4:
> > > > > - Remove enum bootcount_context and replace it with checking
> > > > > gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is
> > > > > cleared in u-boot proper, so can be used as indication if we
> > > > > are in u-boot or SPL).
> > > > > - Do not call bootcount_store() twice when it is not needed.
> > > > > - Call env_set_ulong("bootcount", bootcount); only in NON SPL
> > > > > context - Boards with TINY_PRINTF (in newest mainline) will
> > > > > build break as this
> > > > function
> > > > >    requires simple_itoa() from vsprintf.c (now not always build
> > > > > in SPL).
> > > >
> > > > > Changes in v3:
> > > > > - Unify those functions to also work with common/autoboot.c code
> > > > > - Add enum bootcount_context to distinguish between u-boot
> > > > > proper and SPL
> > > >
> > > > > Changes in v2:
> > > > > - None
> > > >
> > > > >   include/bootcount.h | 47
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > > > 47 insertions(+)
> > > >
> > > > > diff --git a/include/bootcount.h b/include/bootcount.h
> > > > > index e3b3f7028e..a886c98f44 100644
> > > > > --- a/include/bootcount.h
> > > > > +++ b/include/bootcount.h
> > > > > @@ -11,6 +11,8 @@
> > > > >   #include <asm/io.h>
> > > > >   #include <asm/byteorder.h>
> > > >
> > > > > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> > > > defined(CONFIG_BOOTCOUNT_LIMIT)
> > > > > +
> > > > >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
> > > > !defined(CONFIG_SYS_BOOTCOUNT_BE)
> > > > >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> > > > >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > > > > @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile
> > > > > u32
> > > > *addr)
> > > > >          return in_be32(addr);
> > > > >   }
> > > > >   #endif
> > > > > +
> > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > +static inline int 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.",
> > > > > bootlimit);
> > > > > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > > > > +                       printf(" Using altbootcmd.");
> > > > > +               printf("\n");
> > > > > +
> > > > > +               return 1;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static inline void bootcount_inc(void)
> > > > > +{
> > > > > +       unsigned long bootcount = bootcount_load();
> > > > > +
> > > > > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > > > > +               bootcount_store(++bootcount);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > +       /* Only increment bootcount when no bootcount support in
> > > > > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > > > > +       bootcount_store(++bootcount);
> > > > > +#endif
> > > > > +       env_set_ulong("bootcount", bootcount);
> > > > > +#endif /* !CONFIG_SPL_BUILD */
> > > > > +}
> > > > > +
> > > >
> > > > I'm kinda confused by this code... isn't this equivalent.?
> > > >
> > > >    static inline void bootcount_inc(void)
> > > >    {
> > > >           unsigned long bootcount = bootcount_load();
> > > >
> > > >           bootcount_store(++bootcount);
> > > >    #ifndef CONFIG_SPL_BUILD
> > > >           env_set_ulong("bootcount", bootcount);
> > > >    #endif /* !CONFIG_SPL_BUILD */
> > > >    }
> > > >
> > > > Also I suspect bootcount_store() will fail at link time on boards
> > > > where the bootcount is stored in ext4?
> >
> > > I've run this patch set several times with travis-CI. No errors were
> > > present (the travis-ci link is in the cover letter).
> >
> > > Maybe there are some out of tree boards, which use the bootcount in
> > > some "odd" way....
> >
> >
> > I'm only really aware of the ext4 stuff from when I picked through it
> > to consolidate the bootcount into Kconfig. I don't think there would
> > be any Travis failures for this case as you need to have bootcount in
> > SPL which you can't have before this series.

> Could you be more specific here?

> This series adds support for bootcount in SPL. The travis-CI tests
> which I've performed were correct for the all boards which use it:

> https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971

> The bootcount_inc() is added in generic SPL code - this seems to work
> on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT will
> just return from it.

> However, I don't know how the code will behave if one would like to add
> ext4 support to it (including ext4 support to SPL).

> I suppose that those boards will not enable SPL BOOTCOUNT in SPL and
> use the code as it is now - just in u-boot.

> This patch series was designed with one main use case in mind - the
> "falcon" boot of Linux from SPL with bootcount support.

> > If I try building a
> > board like this (choose ext4 for the bootcount) and it fails to link:
> >
> > drivers/built-in.o: In function `bootcount_store':
> > u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to
> > `fs_set_blk_dev'
> > u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to
> > `fs_write'
> > drivers/built-in.o: In function `bootcount_load':
> > u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to
> > `fs_set_blk_dev'
> > u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to
> > `fs_read'
> >
> > But having just played around with Kconfig a bit for this case, I'm
> > not sure there's anything that's actually any better than a link time
> > error; anything we did in Kconfig would just end up modelling out
> > that link error.
> >

> Could you share which particular board this breaks? I've tested it on
> Beagle Bone Black (and display5).


There isn't one (at least not one I'm aware of), but this gives another
impossible combination which you can select, of which we've lots already.

I should've played around some more before commenting as I think what
you've got is everything that's needed.

--
Alex Kiernan

[U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Lukasz Majewski]

Hi Alex,

> On Tue, May 8, 2018 at 10:21 AM Lukasz Majewski <lukma@denx.de> wrote:
> 
> > Hi Alex,  
> 
> > > On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski <lukma@denx.de>
> > > wrote: 
> > > > On Tue, 08 May 2018 05:15:13 +0000
> > > > Alex Kiernan <alex.kiernan@gmail.com> wrote:  
> > >  
> > > > > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma@denx.de>
> > > > > wrote:  
> > > > > > Those two functions can be used to provide easy bootcount
> > > > > > management.  
> > > > >  
> > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> > > > >  
> > > > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > > > > ---  
> > > > >  
> > > > > > Changes in v5:
> > > > > > - Provide parenthesis for #if defined(FOO) && ...  
> > > > >  
> > > > > > Changes in v4:
> > > > > > - Remove enum bootcount_context and replace it with checking
> > > > > > gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is
> > > > > > cleared in u-boot proper, so can be used as indication if we
> > > > > > are in u-boot or SPL).
> > > > > > - Do not call bootcount_store() twice when it is not needed.
> > > > > > - Call env_set_ulong("bootcount", bootcount); only in NON
> > > > > > SPL context - Boards with TINY_PRINTF (in newest mainline)
> > > > > > will build break as this  
> > > > > function  
> > > > > >    requires simple_itoa() from vsprintf.c (now not always
> > > > > > build in SPL).  
> > > > >  
> > > > > > Changes in v3:
> > > > > > - Unify those functions to also work with common/autoboot.c
> > > > > > code
> > > > > > - Add enum bootcount_context to distinguish between u-boot
> > > > > > proper and SPL  
> > > > >  
> > > > > > Changes in v2:
> > > > > > - None  
> > > > >  
> > > > > >   include/bootcount.h | 47
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > > changed, 47 insertions(+)  
> > > > >  
> > > > > > diff --git a/include/bootcount.h b/include/bootcount.h
> > > > > > index e3b3f7028e..a886c98f44 100644
> > > > > > --- a/include/bootcount.h
> > > > > > +++ b/include/bootcount.h
> > > > > > @@ -11,6 +11,8 @@
> > > > > >   #include <asm/io.h>
> > > > > >   #include <asm/byteorder.h>  
> > > > >  
> > > > > > +#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||  
> > > > > defined(CONFIG_BOOTCOUNT_LIMIT)  
> > > > > > +
> > > > > >   #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&  
> > > > > !defined(CONFIG_SYS_BOOTCOUNT_BE)  
> > > > > >   # if __BYTE_ORDER == __LITTLE_ENDIAN
> > > > > >   #  define CONFIG_SYS_BOOTCOUNT_LE
> > > > > > @@ -40,4 +42,49 @@ static inline u32
> > > > > > raw_bootcount_load(volatile u32  
> > > > > *addr)  
> > > > > >          return in_be32(addr);
> > > > > >   }
> > > > > >   #endif
> > > > > > +
> > > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > > +static inline int 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.",
> > > > > > bootlimit);
> > > > > > +               if (!(gd->flags & GD_FLG_SPL_INIT))
> > > > > > +                       printf(" Using altbootcmd.");
> > > > > > +               printf("\n");
> > > > > > +
> > > > > > +               return 1;
> > > > > > +       }
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static inline void bootcount_inc(void)
> > > > > > +{
> > > > > > +       unsigned long bootcount = bootcount_load();
> > > > > > +
> > > > > > +       if (gd->flags & GD_FLG_SPL_INIT) {
> > > > > > +               bootcount_store(++bootcount);
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > +       /* Only increment bootcount when no bootcount
> > > > > > support in SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > > > > > +       bootcount_store(++bootcount);
> > > > > > +#endif
> > > > > > +       env_set_ulong("bootcount", bootcount);
> > > > > > +#endif /* !CONFIG_SPL_BUILD */
> > > > > > +}
> > > > > > +  
> > > > >
> > > > > I'm kinda confused by this code... isn't this equivalent.?
> > > > >
> > > > >    static inline void bootcount_inc(void)
> > > > >    {
> > > > >           unsigned long bootcount = bootcount_load();
> > > > >
> > > > >           bootcount_store(++bootcount);
> > > > >    #ifndef CONFIG_SPL_BUILD
> > > > >           env_set_ulong("bootcount", bootcount);
> > > > >    #endif /* !CONFIG_SPL_BUILD */
> > > > >    }
> > > > >
> > > > > Also I suspect bootcount_store() will fail at link time on
> > > > > boards where the bootcount is stored in ext4?  
> > >  
> > > > I've run this patch set several times with travis-CI. No errors
> > > > were present (the travis-ci link is in the cover letter).  
> > >  
> > > > Maybe there are some out of tree boards, which use the
> > > > bootcount in some "odd" way....  
> > >
> > >
> > > I'm only really aware of the ext4 stuff from when I picked
> > > through it to consolidate the bootcount into Kconfig. I don't
> > > think there would be any Travis failures for this case as you
> > > need to have bootcount in SPL which you can't have before this
> > > series.  
> 
> > Could you be more specific here?  
> 
> > This series adds support for bootcount in SPL. The travis-CI tests
> > which I've performed were correct for the all boards which use it:  
> 
> > https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971  
> 
> > The bootcount_inc() is added in generic SPL code - this seems to
> > work on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT
> > will just return from it.  
> 
> > However, I don't know how the code will behave if one would like to
> > add ext4 support to it (including ext4 support to SPL).  
> 
> > I suppose that those boards will not enable SPL BOOTCOUNT in SPL and
> > use the code as it is now - just in u-boot.  
> 
> > This patch series was designed with one main use case in mind - the
> > "falcon" boot of Linux from SPL with bootcount support.  
> 
> > > If I try building a
> > > board like this (choose ext4 for the bootcount) and it fails to
> > > link:
> > >
> > > drivers/built-in.o: In function `bootcount_store':
> > > u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference
> > > to `fs_set_blk_dev'
> > > u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference
> > > to `fs_write'
> > > drivers/built-in.o: In function `bootcount_load':
> > > u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference
> > > to `fs_set_blk_dev'
> > > u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference
> > > to `fs_read'
> > >
> > > But having just played around with Kconfig a bit for this case,
> > > I'm not sure there's anything that's actually any better than a
> > > link time error; anything we did in Kconfig would just end up
> > > modelling out that link error.
> > >  
> 
> > Could you share which particular board this breaks? I've tested it
> > on Beagle Bone Black (and display5).  
> 
> 
> There isn't one (at least not one I'm aware of), but this gives
> another impossible combination which you can select, of which we've
> lots already.
> 
> I should've played around some more before commenting as I think what
> you've got is everything that's needed.

It is all the matter of use-case.

For SPL I've assumed that we will use IRAM or some RTC special register
(as it is with display5) to store the bootcount (all info - magic
number with boot count in 32 bits).

This seems logical and optimal - since we "assume" that SPL shall be
possible small.

To avoid coincidence one needs to define CONFIG_SPL_BOOTCOUNT.
Without it we shall have the same behaviour as previously on all
supported boards (in theory at least :-) ).

> 
> --
> Alex Kiernan




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

[U-Boot] [U-Boot, v5, 1/7] bootcount: spl: Enable bootcount support in SPL

[Tom Rini]

On Wed, May 02, 2018 at 04:10:50PM +0200, Lukasz Majewski wrote:

> 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>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Alex Kiernan <alex.kiernan@gmail.com>

Applied to u-boot/master, 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/20180511/d02f8c4f/attachment.sig>

[U-Boot] [U-Boot, v5, 2/7] bootcount: Add include guards into bootcount.h file

[Tom Rini]

On Wed, May 02, 2018 at 04:10:51PM +0200, Lukasz Majewski wrote:

> This patch adds missing include guards for bootcount.h file.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Alex Kiernan <alex.kiernan@gmail.com>

Applied to u-boot/master, 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/20180511/29b34604/attachment.sig>

[U-Boot] [U-Boot, v5, 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Tom Rini]

On Wed, May 02, 2018 at 04:10:52PM +0200, Lukasz Majewski wrote:

> Those two functions can be used to provide easy bootcount management.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, 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/20180511/a02ae71e/attachment.sig>

[U-Boot] [U-Boot, v5, 4/7] bootcount: Rewrite autoboot to use wrapper functions from bootcount.h

[Tom Rini]

On Wed, May 02, 2018 at 04:10:53PM +0200, Lukasz Majewski wrote:

> The code has been refactored to use common wrappers from bootcount.h
> header.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by:  Alex Kiernan <alex.kiernan@gmail.com>

Applied to u-boot/master, 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/20180511/a58093c1/attachment.sig>

[U-Boot] [U-Boot, v5, 5/7] bootcount: spl: Extend SPL to support bootcount incrementation

[Tom Rini]

On Wed, May 02, 2018 at 04:10:54PM +0200, Lukasz Majewski wrote:

> This patch adds support for incrementation of the bootcount in SPL.
> Such feature is necessary when we do want to use this feature with
> 'falcon' boot mode (which loads OS directly in SPL).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, 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/20180511/08703d99/attachment.sig>

[U-Boot] [U-Boot, v5, 6/7] bootcount: display5: spl: Extend DISPLAY5 board SPL to support bootcount checking

[Tom Rini]

On Wed, May 02, 2018 at 04:10:55PM +0200, Lukasz Majewski wrote:

> This patch is necessary for providing basic bootcount checking in the case
> of using "falcon" boot mode in that board.
> 
> It forces u-boot proper boot, when we exceed the number of errors.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, 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/20180511/c2902733/attachment.sig>

[U-Boot] [U-Boot, v5, 7/7] bootcount: display5: config: Enable boot count feature in the display5 board

[Tom Rini]

On Wed, May 02, 2018 at 04:10:56PM +0200, Lukasz Majewski wrote:

> The boot count is enabled in both SPL and proper u-boot.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, 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/20180511/7c741185/attachment.sig>