Hi Martin, Ian,

On 01/11/2017 15:23, Martyn Welch wrote:
> From: Ian Ray <[email protected]>
> 
> Add support for bootcounter on an EXT filesystem.
> Sync configuration whitelist.
> 
> Signed-off-by: Ian Ray <[email protected]>
> Signed-off-by: Martyn Welch <[email protected]>
> ---
> Changes for v2:
>    - Adding Kconfig for EXT bootcount.
> 
> Changes for v3:
>    - Add over-arching BOOTCOUNT Kconfig entry.
> 

Should we better split this into two ? The patch patch does more as
commit message suggests. First, it moves bootcounter to Kconfig (first
topic). Second, it adds EXT support.

>  README                            |  7 +++++
>  drivers/Kconfig                   |  2 ++
>  drivers/bootcount/Kconfig         | 57 +++++++++++++++++++++++++++++++++++++
>  drivers/bootcount/Makefile        |  1 +
>  drivers/bootcount/bootcount_ext.c | 59 
> +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 126 insertions(+)
>  create mode 100644 drivers/bootcount/Kconfig
>  create mode 100644 drivers/bootcount/bootcount_ext.c
> 
> diff --git a/README b/README
> index f288176..6336c5c 100644
> --- a/README
> +++ b/README
> @@ -2362,6 +2362,13 @@ The following options need to be configured:
>                       CONFIG_SYS_BOOTCOUNT_ADDR = i2c addr which is used for
>                                                   the bootcounter.
>                       CONFIG_BOOTCOUNT_ALEN = address len
> +             CONFIG_BOOTCOUNT_EXT
> +             enable support for the bootcounter in EXT filesystem
> +                     CONFIG_SYS_BOOTCOUNT_ADDR = RAM address used for read
> +                                                    and write.
> +                     CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE = interface
> +                     CONFIG_SYS_BOOTCOUNT_EXT_DEVPART = device and part
> +                     CONFIG_SYS_BOOTCOUNT_EXT_NAME = filename
>  
>  - Show boot progress:
>               CONFIG_SHOW_BOOT_PROGRESS
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 613e602..c2e813f 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -10,6 +10,8 @@ source "drivers/ata/Kconfig"
>  
>  source "drivers/block/Kconfig"
>  
> +source "drivers/bootcount/Kconfig"
> +
>  source "drivers/clk/Kconfig"
>  
>  source "drivers/cpu/Kconfig"
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> new file mode 100644
> index 0000000..acffbcf
> --- /dev/null
> +++ b/drivers/bootcount/Kconfig
> @@ -0,0 +1,57 @@
> +#
> +# Boot count configuration
> +#
> +
> +menu "Boot count support"
> +
> +config BOOTCOUNT
> +     bool "Enable Boot count support"
> +     help
> +       Enable boot count support, which provides the ability to store the
> +       number of times the board has booted on a number of different
> +       persistent storage mediums.
> +
> +if BOOTCOUNT
> +
> +config BOOTCOUNT_EXT
> +     bool "Boot counter on EXT filesystem"
> +     help
> +       Add support for bootcounter on an EXT filesystem.
> +
> +if BOOTCOUNT_EXT
> +
> +config SYS_BOOTCOUNT_EXT_INTERFACE
> +     string "Interface on which to find boot counter EXT filesystem"
> +     default "mmc"
> +     depends on BOOTCOUNT_EXT
> +     help
> +       Set the interface to use when locating the filesystem to use for the
> +       boot counter.
> +
> +config SYS_BOOTCOUNT_EXT_DEVPART
> +     string "Partition of the boot counter EXT filesystem"
> +     default "0:1"
> +     depends on BOOTCOUNT_EXT
> +     help
> +       Set the partition to use when locating the filesystem to use for the
> +       boot counter.
> +
> +config SYS_BOOTCOUNT_EXT_NAME
> +     string "Path and filename of the EXT filesystem based boot counter"
> +     default "/boot/failures"
> +     depends on BOOTCOUNT_EXT
> +     help
> +       Set the filename and path of the file used to store the boot counter.
> +
> +config SYS_BOOTCOUNT_ADDR
> +     hex "RAM address used for reading and writing the boot counter"
> +     default 0x7000A000
> +     depends on BOOTCOUNT_EXT
> +     help
> +       Set the address used for reading and writing the boot counter.
> +
> +endif
> +
> +endif
> +
> +endmenu
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index ed9659a..45445d2 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_BOOTCOUNT_AM33XX)        += bootcount_davinci.o
>  obj-$(CONFIG_BOOTCOUNT_RAM)  += bootcount_ram.o
>  obj-$(CONFIG_BOOTCOUNT_ENV)  += bootcount_env.o
>  obj-$(CONFIG_BOOTCOUNT_I2C)  += bootcount_i2c.o
> +obj-$(CONFIG_BOOTCOUNT_EXT)  += bootcount_ext.o
> diff --git a/drivers/bootcount/bootcount_ext.c 
> b/drivers/bootcount/bootcount_ext.c
> new file mode 100644
> index 0000000..d36bb0e
> --- /dev/null
> +++ b/drivers/bootcount/bootcount_ext.c
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (c) 2017 General Electric Company. All rights reserved.
> + *
> + * SPDX-License-Identifier:  GPL-2.0+
> + */
> +
> +#include <bootcount.h>
> +#include <fs.h>
> +#include <mapmem.h>
> +
> +#define BC_MAGIC     0xbc
> +
> +void bootcount_store(ulong a)
> +{
> +     uint8_t *buf;
> +     loff_t len;
> +     int ret;
> +
> +     if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE, 
> CONFIG_SYS_BOOTCOUNT_EXT_DEVPART, FS_TYPE_EXT)) {
> +             puts("Error selecting device\n");
> +             return;
> +     }
> +
> +     buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
> +     buf[0] = BC_MAGIC;
> +     buf[1] = (a & 0xff);
> +     unmap_sysmem(buf);
> +
> +     ret = fs_write(CONFIG_SYS_BOOTCOUNT_EXT_NAME, 
> CONFIG_SYS_BOOTCOUNT_ADDR, 0, 2, &len);
> +     if (ret != 0) {
> +             puts("Error storing bootcount\n");
> +     }
> +}
> +
> +ulong bootcount_load(void)
> +{
> +     uint8_t *buf;
> +     loff_t len_read;
> +     int ret;
> +
> +     if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE, 
> CONFIG_SYS_BOOTCOUNT_EXT_DEVPART, FS_TYPE_EXT)) {
> +             puts("Error selecting device\n");
> +             return 0;
> +     }
> +
> +     ret = fs_read(CONFIG_SYS_BOOTCOUNT_EXT_NAME, CONFIG_SYS_BOOTCOUNT_ADDR, 
> 0, 2, &len_read);
> +     if (ret != 0 || len_read != 2) {
> +             puts("Error loading bootcount\n");
> +             return 0;
> +     }
> +
> +     buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
> +     if (buf[0] == BC_MAGIC) {
> +             ret = buf[1];
> +     }
> +     unmap_sysmem(buf);
> +
> +     return ret;
> +}

No comment about the implementation, I just asking why we need a further
way for the bootcounter. IMHO the best way remains putting  it into a
non-persistent storage (RAM / SOC), even if this has the drawback that
is SOC specific. I know there was a discussion about this. Near the RAM
option, there is the bootcounter in ENV, and this is SOC-unaware. Adding
further methods look to me just a work-around to not add u-boot ENV
utils in user space.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: [email protected]
=====================================================================
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to