[U-Boot] [PATCH v1 0/5] Provide SPL support for bootcount (in the case of using falcon boot mode)
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(-)
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/
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 */
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
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);
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.
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@denx.de
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!
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@denx.de
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(); }
Hi Lukasz,
On 26.02.2018 13:22, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de
board/liebherr/display5/spl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -194,6 +195,9 @@ void board_init_f(ulong dummy) /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);
- /* Increment bootcount */
- bootcount_inc();
Wouldn't it be better, to move this incrementing into some common code, so that other platform who might use the bootcounter in SPL in the future could also use it without code duplication?
What do you think?
Thanks, Stefan
Hi Stefan,
Hi Lukasz,
On 26.02.2018 13:22, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de
board/liebherr/display5/spl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -194,6 +195,9 @@ void board_init_f(ulong dummy) /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);
- /* Increment bootcount */
- bootcount_inc();
Wouldn't it be better, to move this incrementing into some common code, so that other platform who might use the bootcounter in SPL in the future could also use it without code duplication?
What do you think?
To provide this functionality I need to add bootcount_inc() and check bootcount_error(). Both are static inlines from #include <bootcount.h>
However, maybe it would be better to put this code to generic SPL. I will try to do it for v2.
Thanks for the idea.
Thanks, Stefan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Mon, 26 Feb 2018 16:22:25 +0100 Lukasz Majewski lukma@denx.de wrote:
Hi Stefan,
Hi Lukasz,
On 26.02.2018 13:22, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de
board/liebherr/display5/spl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -194,6 +195,9 @@ void board_init_f(ulong dummy) /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);
- /* Increment bootcount */
- bootcount_inc();
Wouldn't it be better, to move this incrementing into some common code, so that other platform who might use the bootcounter in SPL in the future could also use it without code duplication?
What do you think?
To provide this functionality I need to add bootcount_inc() and check bootcount_error(). Both are static inlines from #include <bootcount.h>
However, maybe it would be better to put this code to generic SPL. I will try to do it for v2.
The problem here is that we need very early code to do this (increment bootcount). Now it is done in SPL's board_init_f. It is the first C function, called from crt0.S asm file.
I'm not sure if we have such early code common for all archs/platforms.
If we put this bootcount_inc() to latter SPL code, we risk hanging in SPL with WDT reset and not incremented bootcount...
Also the bootcount shall be incremented in the same function where we enable and initialize WDT.
Thanks for the idea.
Thanks, Stefan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi Lukasz,
On 26.02.2018 16:31, Lukasz Majewski wrote:
On Mon, 26 Feb 2018 16:22:25 +0100 Lukasz Majewski lukma@denx.de wrote:
Hi Stefan,
Hi Lukasz,
On 26.02.2018 13:22, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de
board/liebherr/display5/spl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -194,6 +195,9 @@ void board_init_f(ulong dummy) /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);
- /* Increment bootcount */
- bootcount_inc();
Wouldn't it be better, to move this incrementing into some common code, so that other platform who might use the bootcounter in SPL in the future could also use it without code duplication?
What do you think?
To provide this functionality I need to add bootcount_inc() and check bootcount_error(). Both are static inlines from #include <bootcount.h>
However, maybe it would be better to put this code to generic SPL. I will try to do it for v2.
The problem here is that we need very early code to do this (increment bootcount). Now it is done in SPL's board_init_f. It is the first C function, called from crt0.S asm file.
And why does it need to be done "very early" in the SPL? In U-Boot proper, its not done very early but only after all is initialized - so quite late in the boot process. Why should this be different for SPL? Why not increment the bootcounter somewhere in board_init_r() (common/spl/spl.c)?
I'm not sure if we have such early code common for all archs/platforms.
If we put this bootcount_inc() to latter SPL code, we risk hanging in SPL with WDT reset and not incremented bootcount...
So you have a very fast watchdog on your system?
Also the bootcount shall be incremented in the same function where we enable and initialize WDT.
Hmmm. This is also different from U-Boot proper, AFAIK. Where is your watchdog now initialized in SPL? You should be okay, if the watchdog is enabled late in the SPL boot stage. Or am I missing something here?
Thanks, Stefan
Hi Stefan,
Hi Lukasz,
On 26.02.2018 16:31, Lukasz Majewski wrote:
On Mon, 26 Feb 2018 16:22:25 +0100 Lukasz Majewski lukma@denx.de wrote:
Hi Stefan,
Hi Lukasz,
On 26.02.2018 13:22, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de
board/liebherr/display5/spl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 0a36e656c0..5812f71dfc 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -194,6 +195,9 @@ void board_init_f(ulong dummy) /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);
- /* Increment bootcount */
- bootcount_inc();
Wouldn't it be better, to move this incrementing into some common code, so that other platform who might use the bootcounter in SPL in the future could also use it without code duplication?
What do you think?
To provide this functionality I need to add bootcount_inc() and check bootcount_error(). Both are static inlines from #include <bootcount.h>
However, maybe it would be better to put this code to generic SPL. I will try to do it for v2.
The problem here is that we need very early code to do this (increment bootcount). Now it is done in SPL's board_init_f. It is the first C function, called from crt0.S asm file.
And why does it need to be done "very early" in the SPL? In U-Boot proper, its not done very early but only after all is initialized - so quite late in the boot process. Why should this be different for SPL? Why not increment the bootcounter somewhere in board_init_r() (common/spl/spl.c)?
I think I got the point.
Even if we break "early" in SPL, we will not be able to recover as SPL requires u-boot proper to take any action.
I'm not sure if we have such early code common for all archs/platforms.
If we put this bootcount_inc() to latter SPL code, we risk hanging in SPL with WDT reset and not incremented bootcount...
So you have a very fast watchdog on your system?
No. I do use 60s.
Also the bootcount shall be incremented in the same function where we enable and initialize WDT.
Hmmm. This is also different from U-Boot proper, AFAIK.
Yes. This is different. We increment bootcount when we do have everything setup (just before we count down with "bootdelay"). WDT is setup earlier.
Where is your watchdog now initialized in SPL?
WDT is setup in board_init_f. Just after providing clocks.
You should be okay, if the watchdog is enabled late in the SPL boot stage. Or am I missing something here?
No. I think that it would be safe to move the WDT and bootcount latter in SPL.
Thanks, Stefan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
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
participants (3)
-
Lukasz Majewski -
Stefan Roese -
Tom Rini