[PATCH v2 0/1]: drivers: bootcount: Add support for FAT filesystem

[PATCH v2 0/1]: drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

Changes in v2:
	- Use generic code by utilizing FS_TYPE_ANY
	- Change filename from bootcount_ext.c to bootcount_fs.c

v1: https://lore.kernel.org/u-boot/20240605181041.68317-1-vassilisamir@gmail.com/

This patch adds support to save the bootcount variable in a file located in
FAT filesystem. Up to now, there was support only for EXT filesystem.

The reason I put this as RFC is because the file "bootcount_ext.c" will no
longer represent the implementation for the EXT filesystem, but also for the
FAT filesystem. Should it be renamed? If not, wouldn't it be inconsistent?

Cheers,
Vasilis

P.S: Documentation has not been modified yet, I will submit in the v2 (if any).

Vasileios Amoiridis (1):
  drivers: bootcount: Add support for FAT filesystem

 doc/README.bootcount                          | 12 ++---
 drivers/bootcount/Kconfig                     | 53 +++++++++++++------
 drivers/bootcount/Makefile                    |  2 +-
 .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
 4 files changed, 50 insertions(+), 29 deletions(-)
 rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)

-- 
2.25.1

[PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

Add support to save boot count variable in a file in a FAT filesystem.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 doc/README.bootcount                          | 12 ++---
 drivers/bootcount/Kconfig                     | 53 +++++++++++++------
 drivers/bootcount/Makefile                    |  2 +-
 .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
 4 files changed, 50 insertions(+), 29 deletions(-)
 rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)

diff --git a/doc/README.bootcount b/doc/README.bootcount
index f6c5f82f..0f4ffb68 100644
--- a/doc/README.bootcount
+++ b/doc/README.bootcount
@@ -23,15 +23,15 @@ It is the responsibility of some application code (typically a Linux
 application) to reset the variable "bootcount" to 0 when the system booted
 successfully, thus allowing for more boot cycles.
 
-CONFIG_BOOTCOUNT_EXT
+CONFIG_BOOTCOUNT_FS
 --------------------
 
-This adds support for maintaining boot count in a file on an EXT filesystem.
-The file to use is defined by:
+This adds support for maintaining boot count in a file on a filesystem.
+Supported filesystems are FAT and EXT. The file to use is defined by:
 
-CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
-CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
-CONFIG_SYS_BOOTCOUNT_EXT_NAME
+CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
+CONFIG_SYS_BOOTCOUNT_FS_DEVPART
+CONFIG_SYS_BOOTCOUNT_FS_NAME
 
 The format of the file is:
 
diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index 3c56253b..d3679eb5 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
 	    Set to the address where the bootcount and bootcount magic
 	    will be stored.
 
-config BOOTCOUNT_EXT
-	bool "Boot counter on EXT filesystem"
-	depends on FS_EXT4
-	select EXT4_WRITE
+config BOOTCOUNT_FS
+	bool "Boot counter on a filesystem"
+	depends on FS_EXT4 || FS_FAT
 	help
 	  Add support for maintaining boot count in a file on an EXT
 	  filesystem.
@@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
 	  This option enables packing boot count magic value and boot count
 	  into single word (32 bits).
 
-config SYS_BOOTCOUNT_EXT_INTERFACE
-	string "Interface on which to find boot counter EXT filesystem"
+if BOOTCOUNT_FS
+choice
+	prompt "Filesystem type"
+	default BOOTCOUNT_EXT
+
+config BOOTCOUNT_EXT
+	bool "Boot counter on EXT filesystem"
+	depends on FS_EXT4
+	select EXT4_WRITE
+	help
+	  Add support for maintaing boot counter in a file on EXT filesystem"
+
+config BOOTCOUNT_FAT
+	bool "Boot counter on FAT filesystem"
+	depends on FS_FAT
+	select FAT_WRITE
+	help
+	  Add support for maintaing boot counter in a file on FAT filesystem"
+
+endchoice
+endif
+
+config SYS_BOOTCOUNT_FS_INTERFACE
+	string "Interface on which to find boot counter filesystem"
 	default "mmc"
-	depends on BOOTCOUNT_EXT
+	depends on BOOTCOUNT_FS
 	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"
+config SYS_BOOTCOUNT_FS_DEVPART
+	string "Partition of the boot counter filesystem"
 	default "0:1"
-	depends on BOOTCOUNT_EXT
+	depends on BOOTCOUNT_FS
 	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"
+config SYS_BOOTCOUNT_FS_NAME
+	string "Path and filename of the FS filesystem based boot counter"
 	default "/boot/failures"
-	depends on BOOTCOUNT_EXT
+	depends on BOOTCOUNT_FS
 	help
 	  Set the filename and path of the file used to store the boot counter.
 
@@ -211,18 +232,18 @@ config SYS_BOOTCOUNT_ADDR
 	hex "RAM address used for reading and writing the boot counter"
 	default 0x44E3E000 if BOOTCOUNT_AM33XX || BOOTCOUNT_AM33XX_NVMEM
 	default 0xE0115FF8 if ARCH_LS1043A || ARCH_LS1021A
-	depends on BOOTCOUNT_AM33XX || BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
+	depends on BOOTCOUNT_AM33XX || BOOTCOUNT_GENERIC || BOOTCOUNT_FS || \
 		   BOOTCOUNT_AM33XX_NVMEM
 	help
 	  Set the address used for reading and writing the boot counter.
 
 config SYS_BOOTCOUNT_MAGIC
 	hex "Magic value for the boot counter"
-	default 0xB001C041 if BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
+	default 0xB001C041 if BOOTCOUNT_GENERIC || BOOTCOUNT_FS || \
 			      BOOTCOUNT_AM33XX || BOOTCOUNT_ENV || \
 			      BOOTCOUNT_RAM || BOOTCOUNT_AT91 || DM_BOOTCOUNT
 	default 0xB0 if BOOTCOUNT_AM33XX_NVMEM
-	depends on BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
+	depends on BOOTCOUNT_GENERIC || BOOTCOUNT_FS || \
 		   BOOTCOUNT_AM33XX || BOOTCOUNT_ENV || \
 		   BOOTCOUNT_RAM || BOOTCOUNT_AT91 || DM_BOOTCOUNT || \
 		   BOOTCOUNT_AM33XX_NVMEM
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index e7771f5b..245f8796 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
 obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o
 obj-$(CONFIG_BOOTCOUNT_ENV)	+= bootcount_env.o
-obj-$(CONFIG_BOOTCOUNT_EXT)	+= bootcount_ext.o
+obj-$(CONFIG_BOOTCOUNT_FS)	+= bootcount_fs.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX_NVMEM)	+= bootcount_nvmem.o
 
 obj-$(CONFIG_DM_BOOTCOUNT)      += bootcount-uclass.o
diff --git a/drivers/bootcount/bootcount_ext.c b/drivers/bootcount/bootcount_fs.c
similarity index 81%
rename from drivers/bootcount/bootcount_ext.c
rename to drivers/bootcount/bootcount_fs.c
index 9639e638..569592d8 100644
--- a/drivers/bootcount/bootcount_ext.c
+++ b/drivers/bootcount/bootcount_fs.c
@@ -25,8 +25,8 @@ void bootcount_store(ulong a)
 	loff_t len;
 	int ret;
 
-	if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE,
-			   CONFIG_SYS_BOOTCOUNT_EXT_DEVPART, FS_TYPE_EXT)) {
+	if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_FS_INTERFACE,
+			   CONFIG_SYS_BOOTCOUNT_FS_DEVPART, FS_TYPE_ANY)) {
 		puts("Error selecting device\n");
 		return;
 	}
@@ -42,7 +42,7 @@ void bootcount_store(ulong a)
 	buf->upgrade_available = upgrade_available;
 	unmap_sysmem(buf);
 
-	ret = fs_write(CONFIG_SYS_BOOTCOUNT_EXT_NAME,
+	ret = fs_write(CONFIG_SYS_BOOTCOUNT_FS_NAME,
 		       CONFIG_SYS_BOOTCOUNT_ADDR, 0, sizeof(bootcount_ext_t),
 		       &len);
 	if (ret != 0)
@@ -55,13 +55,13 @@ ulong bootcount_load(void)
 	loff_t len_read;
 	int ret;
 
-	if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE,
-			   CONFIG_SYS_BOOTCOUNT_EXT_DEVPART, FS_TYPE_EXT)) {
+	if (fs_set_blk_dev(CONFIG_SYS_BOOTCOUNT_FS_INTERFACE,
+			   CONFIG_SYS_BOOTCOUNT_FS_DEVPART, FS_TYPE_ANY)) {
 		puts("Error selecting device\n");
 		return 0;
 	}
 
-	ret = fs_read(CONFIG_SYS_BOOTCOUNT_EXT_NAME, CONFIG_SYS_BOOTCOUNT_ADDR,
+	ret = fs_read(CONFIG_SYS_BOOTCOUNT_FS_NAME, CONFIG_SYS_BOOTCOUNT_ADDR,
 		      0, sizeof(bootcount_ext_t), &len_read);
 	if (ret != 0 || len_read != sizeof(bootcount_ext_t)) {
 		puts("Error loading bootcount\n");
-- 
2.25.1

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Mon, Jun 10, 2024 at 08:51:16PM +0200, Vasileios Amoiridis wrote:

> Add support to save boot count variable in a file in a FAT filesystem.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

Thanks for doing this.

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

Would you be able to do a follow-up that converts doc/README.bootcount
to rST as say doc/api/bootcount.rst ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

On Mon, Jun 10, 2024 at 01:37:47PM -0600, Tom Rini wrote:
> On Mon, Jun 10, 2024 at 08:51:16PM +0200, Vasileios Amoiridis wrote:
> 
> > Add support to save boot count variable in a file in a FAT filesystem.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Thanks for doing this.
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> Would you be able to do a follow-up that converts doc/README.bootcount
> to rST as say doc/api/bootcount.rst ?
> 
> -- 
> Tom

Hi Tom,

Yes I could do that. I will send a follow-up with the change that you propose.

Cheers,
Vasilis

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Mon, Jun 10, 2024 at 10:03:25PM +0200, Vasileios Amoiridis wrote:
> On Mon, Jun 10, 2024 at 01:37:47PM -0600, Tom Rini wrote:
> > On Mon, Jun 10, 2024 at 08:51:16PM +0200, Vasileios Amoiridis wrote:
> > 
> > > Add support to save boot count variable in a file in a FAT filesystem.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Thanks for doing this.
> > 
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > 
> > Would you be able to do a follow-up that converts doc/README.bootcount
> > to rST as say doc/api/bootcount.rst ?
> > 
> > -- 
> > Tom
> 
> Hi Tom,
> 
> Yes I could do that. I will send a follow-up with the change that you propose.

Great, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Heiko Schocher]

Hello Amoiridis,

On 10.06.24 20:51, Vasileios Amoiridis wrote:
> Add support to save boot count variable in a file in a FAT filesystem.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>   doc/README.bootcount                          | 12 ++---
>   drivers/bootcount/Kconfig                     | 53 +++++++++++++------
>   drivers/bootcount/Makefile                    |  2 +-
>   .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
>   4 files changed, 50 insertions(+), 29 deletions(-)
>   rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)

Thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko


-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Quentin Schulz]

Hi Vasileios,

On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
 > Add support to save boot count variable in a file in a FAT filesystem.
 >
 > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
 > ---
 >   doc/README.bootcount                          | 12 ++---
 >   drivers/bootcount/Kconfig                     | 53 +++++++++++++------
 >   drivers/bootcount/Makefile                    |  2 +-
 >   .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
 >   4 files changed, 50 insertions(+), 29 deletions(-)
 >   rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
 >
 > diff --git a/doc/README.bootcount b/doc/README.bootcount
 > index f6c5f82f..0f4ffb68 100644
 > --- a/doc/README.bootcount
 > +++ b/doc/README.bootcount
 > @@ -23,15 +23,15 @@ It is the responsibility of some application code 
(typically a Linux
 >   application) to reset the variable "bootcount" to 0 when the system 
booted
 >   successfully, thus allowing for more boot cycles.
 >
 > -CONFIG_BOOTCOUNT_EXT
 > +CONFIG_BOOTCOUNT_FS
 >   --------------------
 >
 > -This adds support for maintaining boot count in a file on an EXT 
filesystem.
 > -The file to use is defined by:
 > +This adds support for maintaining boot count in a file on a filesystem.
 > +Supported filesystems are FAT and EXT. The file to use is defined by:
 >
 > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
 > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
 > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
 > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
 > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
 > +CONFIG_SYS_BOOTCOUNT_FS_NAME
 >
 >   The format of the file is:
 >
 > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
 > index 3c56253b..d3679eb5 100644
 > --- a/drivers/bootcount/Kconfig
 > +++ b/drivers/bootcount/Kconfig
 > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
 >   	    Set to the address where the bootcount and bootcount magic
 >   	    will be stored.
 >
 > -config BOOTCOUNT_EXT
 > -	bool "Boot counter on EXT filesystem"
 > -	depends on FS_EXT4
 > -	select EXT4_WRITE
 > +config BOOTCOUNT_FS
 > +	bool "Boot counter on a filesystem"
 > +	depends on FS_EXT4 || FS_FAT
Do we really need this 'depends on' here? Especially if we have a choice 
below...

 >   	help
 >   	  Add support for maintaining boot count in a file on an EXT
The help text is still mentioning EXT here.

I would recommend removing it, or listing the supported filesystems at 
the moment. While I assume you tested with FAT, I assume that with 
FS_ANY, any FS should be supported?

 >   	  filesystem.
 > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
 >   	  This option enables packing boot count magic value and boot count
 >   	  into single word (32 bits).
 >
 > -config SYS_BOOTCOUNT_EXT_INTERFACE
 > -	string "Interface on which to find boot counter EXT filesystem"
 > +if BOOTCOUNT_FS
 > +choice
 > +	prompt "Filesystem type"
 > +	default BOOTCOUNT_EXT
 > +
 > +config BOOTCOUNT_EXT
 > +	bool "Boot counter on EXT filesystem"
 > +	depends on FS_EXT4
 > +	select EXT4_WRITE
 > +	help
 > +	  Add support for maintaing boot counter in a file on EXT filesystem"
 > +
 > +config BOOTCOUNT_FAT
 > +	bool "Boot counter on FAT filesystem"
 > +	depends on FS_FAT
 > +	select FAT_WRITE
 > +	help
 > +	  Add support for maintaing boot counter in a file on FAT filesystem"
 > +
 > +endchoice
 > +endif
 > +
Since we now support FS_ANY, do we really need this choice at all?

Alternatively, should it **really** be a choice and not just a bunch of 
configs that depends on BOOTCOUNT_FS + whatever's needed to write on 
that FS instead? I think we could have both BOOTCOUNT_EXT and 
BOOTCOUNT_FAT set without issue?

Cheers,
Quentin

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> Hi Vasileios,
> 
> On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
> > Add support to save boot count variable in a file in a FAT filesystem.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >   doc/README.bootcount                          | 12 ++---
> >   drivers/bootcount/Kconfig                     | 53 +++++++++++++------
> >   drivers/bootcount/Makefile                    |  2 +-
> >   .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
> >   4 files changed, 50 insertions(+), 29 deletions(-)
> >   rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
> >
> > diff --git a/doc/README.bootcount b/doc/README.bootcount
> > index f6c5f82f..0f4ffb68 100644
> > --- a/doc/README.bootcount
> > +++ b/doc/README.bootcount
> > @@ -23,15 +23,15 @@ It is the responsibility of some application code
> (typically a Linux
> >   application) to reset the variable "bootcount" to 0 when the system
> booted
> >   successfully, thus allowing for more boot cycles.
> >
> > -CONFIG_BOOTCOUNT_EXT
> > +CONFIG_BOOTCOUNT_FS
> >   --------------------
> >
> > -This adds support for maintaining boot count in a file on an EXT
> filesystem.
> > -The file to use is defined by:
> > +This adds support for maintaining boot count in a file on a filesystem.
> > +Supported filesystems are FAT and EXT. The file to use is defined by:
> >
> > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
> > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
> > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
> > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
> > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
> > +CONFIG_SYS_BOOTCOUNT_FS_NAME
> >
> >   The format of the file is:
> >
> > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > index 3c56253b..d3679eb5 100644
> > --- a/drivers/bootcount/Kconfig
> > +++ b/drivers/bootcount/Kconfig
> > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
> >   	    Set to the address where the bootcount and bootcount magic
> >   	    will be stored.
> >
> > -config BOOTCOUNT_EXT
> > -	bool "Boot counter on EXT filesystem"
> > -	depends on FS_EXT4
> > -	select EXT4_WRITE
> > +config BOOTCOUNT_FS
> > +	bool "Boot counter on a filesystem"
> > +	depends on FS_EXT4 || FS_FAT
> Do we really need this 'depends on' here? Especially if we have a choice
> below...
> 
Well, probably this is redundant indeed.

> >   	help
> >   	  Add support for maintaining boot count in a file on an EXT
> The help text is still mentioning EXT here.
> 

Ahh, I missed that.

> I would recommend removing it, or listing the supported filesystems at the
> moment. While I assume you tested with FAT, I assume that with FS_ANY, any
> FS should be supported?
> 

Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
implementation of the filesystem handling code in U-Boot, if the fs supports
a write a function, then it should work. But I cannot test for other
filesystems apart from FAT and EXT4 so I think it's better to limit the
option to these two.

> >   	  filesystem.
> > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
> >   	  This option enables packing boot count magic value and boot count
> >   	  into single word (32 bits).
> >
> > -config SYS_BOOTCOUNT_EXT_INTERFACE
> > -	string "Interface on which to find boot counter EXT filesystem"
> > +if BOOTCOUNT_FS
> > +choice
> > +	prompt "Filesystem type"
> > +	default BOOTCOUNT_EXT
> > +
> > +config BOOTCOUNT_EXT
> > +	bool "Boot counter on EXT filesystem"
> > +	depends on FS_EXT4
> > +	select EXT4_WRITE
> > +	help
> > +	  Add support for maintaing boot counter in a file on EXT filesystem"
> > +
> > +config BOOTCOUNT_FAT
> > +	bool "Boot counter on FAT filesystem"
> > +	depends on FS_FAT
> > +	select FAT_WRITE
> > +	help
> > +	  Add support for maintaing boot counter in a file on FAT filesystem"
> > +
> > +endchoice
> > +endif
> > +
> Since we now support FS_ANY, do we really need this choice at all?
> 
> Alternatively, should it **really** be a choice and not just a bunch of
> configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
> instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
> without issue?
> 
> Cheers,
> Quentin

Well, I think I kind of get the point but I am still a bit confused.
Do you mean that basically the configuration should be done the other way
around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
If yes, what is the advantage of this approach?

Cheers,
Vasilis

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Quentin Schulz]

Hi Vasileios,

On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
>> Hi Vasileios,
>>
>> On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
>>> Add support to save boot count variable in a file in a FAT filesystem.
>>>
>>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
>>> ---
>>>    doc/README.bootcount                          | 12 ++---
>>>    drivers/bootcount/Kconfig                     | 53 +++++++++++++------
>>>    drivers/bootcount/Makefile                    |  2 +-
>>>    .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
>>>    4 files changed, 50 insertions(+), 29 deletions(-)
>>>    rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
>>>
>>> diff --git a/doc/README.bootcount b/doc/README.bootcount
>>> index f6c5f82f..0f4ffb68 100644
>>> --- a/doc/README.bootcount
>>> +++ b/doc/README.bootcount
>>> @@ -23,15 +23,15 @@ It is the responsibility of some application code
>> (typically a Linux
>>>    application) to reset the variable "bootcount" to 0 when the system
>> booted
>>>    successfully, thus allowing for more boot cycles.
>>>
>>> -CONFIG_BOOTCOUNT_EXT
>>> +CONFIG_BOOTCOUNT_FS
>>>    --------------------
>>>
>>> -This adds support for maintaining boot count in a file on an EXT
>> filesystem.
>>> -The file to use is defined by:
>>> +This adds support for maintaining boot count in a file on a filesystem.
>>> +Supported filesystems are FAT and EXT. The file to use is defined by:
>>>
>>> -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
>>> -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
>>> -CONFIG_SYS_BOOTCOUNT_EXT_NAME
>>> +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
>>> +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
>>> +CONFIG_SYS_BOOTCOUNT_FS_NAME
>>>
>>>    The format of the file is:
>>>
>>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>>> index 3c56253b..d3679eb5 100644
>>> --- a/drivers/bootcount/Kconfig
>>> +++ b/drivers/bootcount/Kconfig
>>> @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
>>>    	    Set to the address where the bootcount and bootcount magic
>>>    	    will be stored.
>>>
>>> -config BOOTCOUNT_EXT
>>> -	bool "Boot counter on EXT filesystem"
>>> -	depends on FS_EXT4
>>> -	select EXT4_WRITE
>>> +config BOOTCOUNT_FS
>>> +	bool "Boot counter on a filesystem"
>>> +	depends on FS_EXT4 || FS_FAT
>> Do we really need this 'depends on' here? Especially if we have a choice
>> below...
>>
> Well, probably this is redundant indeed.
> 
>>>    	help
>>>    	  Add support for maintaining boot count in a file on an EXT
>> The help text is still mentioning EXT here.
>>
> 
> Ahh, I missed that.
> 
>> I would recommend removing it, or listing the supported filesystems at the
>> moment. While I assume you tested with FAT, I assume that with FS_ANY, any
>> FS should be supported?
>>
> 
> Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
> implementation of the filesystem handling code in U-Boot, if the fs supports
> a write a function, then it should work. But I cannot test for other
> filesystems apart from FAT and EXT4 so I think it's better to limit the
> option to these two.
> 

I guess we can let people figure things out themselves and add new 
options for when they have tested them, no strong opinion here.

>>>    	  filesystem.
>>> @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
>>>    	  This option enables packing boot count magic value and boot count
>>>    	  into single word (32 bits).
>>>
>>> -config SYS_BOOTCOUNT_EXT_INTERFACE
>>> -	string "Interface on which to find boot counter EXT filesystem"
>>> +if BOOTCOUNT_FS
>>> +choice
>>> +	prompt "Filesystem type"
>>> +	default BOOTCOUNT_EXT
>>> +
>>> +config BOOTCOUNT_EXT
>>> +	bool "Boot counter on EXT filesystem"
>>> +	depends on FS_EXT4
>>> +	select EXT4_WRITE
>>> +	help
>>> +	  Add support for maintaing boot counter in a file on EXT filesystem"
>>> +
>>> +config BOOTCOUNT_FAT
>>> +	bool "Boot counter on FAT filesystem"
>>> +	depends on FS_FAT
>>> +	select FAT_WRITE
>>> +	help
>>> +	  Add support for maintaing boot counter in a file on FAT filesystem"
>>> +

Seems like I missed a typo here as well:

s/maintaing/maintaining/ ? At least that's what we have in 
doc/README.bootcount

>>> +endchoice
>>> +endif
>>> +
>> Since we now support FS_ANY, do we really need this choice at all?
>>
>> Alternatively, should it **really** be a choice and not just a bunch of
>> configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
>> instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
>> without issue?
>>
>> Cheers,
>> Quentin
> 
> Well, I think I kind of get the point but I am still a bit confused.
> Do you mean that basically the configuration should be done the other way
> around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
> EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
> If yes, what is the advantage of this approach?
> 

I'm suggesting:

"""
config BOOTCOUNT_FS
	bool "Boot counter on a filesystem"
  	help

config BOOTCOUNT_EXT
	bool "Boot counter on EXT filesystem"
         default y
	depends on BOOTCOUNT_FS
	depends on FS_EXT4
	select EXT4_WRITE
	help
	  Add support for maintaing boot counter in a file on EXT filesystem"

config BOOTCOUNT_FAT
	bool "Boot counter on FAT filesystem"
	depends on BOOTCOUNT_FS
	depends on FS_FAT
	select FAT_WRITE
	help
	  Add support for maintaing boot counter in a file on FAT filesystem"
"""

This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT 
are both selected, the user would then be free to decide if the same 
partition on two different devices but for the same purpose can be 
either ext2/3/4 or FAT, without recompiling U-Boot just for that.

However, it would now be possible to have BOOTCOUNT_FS=y but neither 
BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT 
isn't defined).

Finally, the other option was just to NOT have BOOTCOUNT_FAT or 
BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and 
EXT4_WRITE/FAT_WRITE themselves since the BOOTCOUNT_FAT/EXT aren't 
actually used in C code. This is less user-friendly though.

Cheers,
Quentin

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7245 bytes --]

On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
> Hi Vasileios,
> 
> On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> > On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> > > Hi Vasileios,
> > > 
> > > On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
> > > > Add support to save boot count variable in a file in a FAT filesystem.
> > > > 
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > ---
> > > >    doc/README.bootcount                          | 12 ++---
> > > >    drivers/bootcount/Kconfig                     | 53 +++++++++++++------
> > > >    drivers/bootcount/Makefile                    |  2 +-
> > > >    .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
> > > >    4 files changed, 50 insertions(+), 29 deletions(-)
> > > >    rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
> > > > 
> > > > diff --git a/doc/README.bootcount b/doc/README.bootcount
> > > > index f6c5f82f..0f4ffb68 100644
> > > > --- a/doc/README.bootcount
> > > > +++ b/doc/README.bootcount
> > > > @@ -23,15 +23,15 @@ It is the responsibility of some application code
> > > (typically a Linux
> > > >    application) to reset the variable "bootcount" to 0 when the system
> > > booted
> > > >    successfully, thus allowing for more boot cycles.
> > > > 
> > > > -CONFIG_BOOTCOUNT_EXT
> > > > +CONFIG_BOOTCOUNT_FS
> > > >    --------------------
> > > > 
> > > > -This adds support for maintaining boot count in a file on an EXT
> > > filesystem.
> > > > -The file to use is defined by:
> > > > +This adds support for maintaining boot count in a file on a filesystem.
> > > > +Supported filesystems are FAT and EXT. The file to use is defined by:
> > > > 
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
> > > > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
> > > > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
> > > > +CONFIG_SYS_BOOTCOUNT_FS_NAME
> > > > 
> > > >    The format of the file is:
> > > > 
> > > > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > > > index 3c56253b..d3679eb5 100644
> > > > --- a/drivers/bootcount/Kconfig
> > > > +++ b/drivers/bootcount/Kconfig
> > > > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
> > > >    	    Set to the address where the bootcount and bootcount magic
> > > >    	    will be stored.
> > > > 
> > > > -config BOOTCOUNT_EXT
> > > > -	bool "Boot counter on EXT filesystem"
> > > > -	depends on FS_EXT4
> > > > -	select EXT4_WRITE
> > > > +config BOOTCOUNT_FS
> > > > +	bool "Boot counter on a filesystem"
> > > > +	depends on FS_EXT4 || FS_FAT
> > > Do we really need this 'depends on' here? Especially if we have a choice
> > > below...
> > > 
> > Well, probably this is redundant indeed.
> > 
> > > >    	help
> > > >    	  Add support for maintaining boot count in a file on an EXT
> > > The help text is still mentioning EXT here.
> > > 
> > 
> > Ahh, I missed that.
> > 
> > > I would recommend removing it, or listing the supported filesystems at the
> > > moment. While I assume you tested with FAT, I assume that with FS_ANY, any
> > > FS should be supported?
> > > 
> > 
> > Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
> > implementation of the filesystem handling code in U-Boot, if the fs supports
> > a write a function, then it should work. But I cannot test for other
> > filesystems apart from FAT and EXT4 so I think it's better to limit the
> > option to these two.
> > 
> 
> I guess we can let people figure things out themselves and add new options
> for when they have tested them, no strong opinion here.
> 
> > > >    	  filesystem.
> > > > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
> > > >    	  This option enables packing boot count magic value and boot count
> > > >    	  into single word (32 bits).
> > > > 
> > > > -config SYS_BOOTCOUNT_EXT_INTERFACE
> > > > -	string "Interface on which to find boot counter EXT filesystem"
> > > > +if BOOTCOUNT_FS
> > > > +choice
> > > > +	prompt "Filesystem type"
> > > > +	default BOOTCOUNT_EXT
> > > > +
> > > > +config BOOTCOUNT_EXT
> > > > +	bool "Boot counter on EXT filesystem"
> > > > +	depends on FS_EXT4
> > > > +	select EXT4_WRITE
> > > > +	help
> > > > +	  Add support for maintaing boot counter in a file on EXT filesystem"
> > > > +
> > > > +config BOOTCOUNT_FAT
> > > > +	bool "Boot counter on FAT filesystem"
> > > > +	depends on FS_FAT
> > > > +	select FAT_WRITE
> > > > +	help
> > > > +	  Add support for maintaing boot counter in a file on FAT filesystem"
> > > > +
> 
> Seems like I missed a typo here as well:
> 
> s/maintaing/maintaining/ ? At least that's what we have in
> doc/README.bootcount
> 
> > > > +endchoice
> > > > +endif
> > > > +
> > > Since we now support FS_ANY, do we really need this choice at all?
> > > 
> > > Alternatively, should it **really** be a choice and not just a bunch of
> > > configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
> > > instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
> > > without issue?
> > > 
> > > Cheers,
> > > Quentin
> > 
> > Well, I think I kind of get the point but I am still a bit confused.
> > Do you mean that basically the configuration should be done the other way
> > around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
> > EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
> > If yes, what is the advantage of this approach?
> > 
> 
> I'm suggesting:
> 
> """
> config BOOTCOUNT_FS
> 	bool "Boot counter on a filesystem"
>  	help
> 
> config BOOTCOUNT_EXT
> 	bool "Boot counter on EXT filesystem"
>         default y
> 	depends on BOOTCOUNT_FS
> 	depends on FS_EXT4
> 	select EXT4_WRITE
> 	help
> 	  Add support for maintaing boot counter in a file on EXT filesystem"
> 
> config BOOTCOUNT_FAT
> 	bool "Boot counter on FAT filesystem"
> 	depends on BOOTCOUNT_FS
> 	depends on FS_FAT
> 	select FAT_WRITE
> 	help
> 	  Add support for maintaing boot counter in a file on FAT filesystem"
> """
> 
> This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT are
> both selected, the user would then be free to decide if the same partition
> on two different devices but for the same purpose can be either ext2/3/4 or
> FAT, without recompiling U-Boot just for that.
> 
> However, it would now be possible to have BOOTCOUNT_FS=y but neither
> BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT isn't
> defined).
> 
> Finally, the other option was just to NOT have BOOTCOUNT_FAT or
> BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and EXT4_WRITE/FAT_WRITE
> themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code. This
> is less user-friendly though.

I was thinking that with FS_ANY, we don't need a symbol per filesystem
type but can just handle it in the help text with something like "Please
ensure that you have enabled write support for the filesystem that you
will be used by the partition that you configure this feature for."

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

On Tue, Jun 11, 2024 at 10:06:59AM -0600, Tom Rini wrote:
> On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
> > Hi Vasileios,
> > 
> > On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> > > On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> > > > Hi Vasileios,
> > > > 
> > > > On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
> > > > > Add support to save boot count variable in a file in a FAT filesystem.
> > > > > 
> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > > ---
> > > > >    doc/README.bootcount                          | 12 ++---
> > > > >    drivers/bootcount/Kconfig                     | 53 +++++++++++++------
> > > > >    drivers/bootcount/Makefile                    |  2 +-
> > > > >    .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
> > > > >    4 files changed, 50 insertions(+), 29 deletions(-)
> > > > >    rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
> > > > > 
> > > > > diff --git a/doc/README.bootcount b/doc/README.bootcount
> > > > > index f6c5f82f..0f4ffb68 100644
> > > > > --- a/doc/README.bootcount
> > > > > +++ b/doc/README.bootcount
> > > > > @@ -23,15 +23,15 @@ It is the responsibility of some application code
> > > > (typically a Linux
> > > > >    application) to reset the variable "bootcount" to 0 when the system
> > > > booted
> > > > >    successfully, thus allowing for more boot cycles.
> > > > > 
> > > > > -CONFIG_BOOTCOUNT_EXT
> > > > > +CONFIG_BOOTCOUNT_FS
> > > > >    --------------------
> > > > > 
> > > > > -This adds support for maintaining boot count in a file on an EXT
> > > > filesystem.
> > > > > -The file to use is defined by:
> > > > > +This adds support for maintaining boot count in a file on a filesystem.
> > > > > +Supported filesystems are FAT and EXT. The file to use is defined by:
> > > > > 
> > > > > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
> > > > > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
> > > > > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
> > > > > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
> > > > > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
> > > > > +CONFIG_SYS_BOOTCOUNT_FS_NAME
> > > > > 
> > > > >    The format of the file is:
> > > > > 
> > > > > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > > > > index 3c56253b..d3679eb5 100644
> > > > > --- a/drivers/bootcount/Kconfig
> > > > > +++ b/drivers/bootcount/Kconfig
> > > > > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
> > > > >    	    Set to the address where the bootcount and bootcount magic
> > > > >    	    will be stored.
> > > > > 
> > > > > -config BOOTCOUNT_EXT
> > > > > -	bool "Boot counter on EXT filesystem"
> > > > > -	depends on FS_EXT4
> > > > > -	select EXT4_WRITE
> > > > > +config BOOTCOUNT_FS
> > > > > +	bool "Boot counter on a filesystem"
> > > > > +	depends on FS_EXT4 || FS_FAT
> > > > Do we really need this 'depends on' here? Especially if we have a choice
> > > > below...
> > > > 
> > > Well, probably this is redundant indeed.
> > > 
> > > > >    	help
> > > > >    	  Add support for maintaining boot count in a file on an EXT
> > > > The help text is still mentioning EXT here.
> > > > 
> > > 
> > > Ahh, I missed that.
> > > 
> > > > I would recommend removing it, or listing the supported filesystems at the
> > > > moment. While I assume you tested with FAT, I assume that with FS_ANY, any
> > > > FS should be supported?
> > > > 
> > > 
> > > Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
> > > implementation of the filesystem handling code in U-Boot, if the fs supports
> > > a write a function, then it should work. But I cannot test for other
> > > filesystems apart from FAT and EXT4 so I think it's better to limit the
> > > option to these two.
> > > 
> > 
> > I guess we can let people figure things out themselves and add new options
> > for when they have tested them, no strong opinion here.
> > 
> > > > >    	  filesystem.
> > > > > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
> > > > >    	  This option enables packing boot count magic value and boot count
> > > > >    	  into single word (32 bits).
> > > > > 
> > > > > -config SYS_BOOTCOUNT_EXT_INTERFACE
> > > > > -	string "Interface on which to find boot counter EXT filesystem"
> > > > > +if BOOTCOUNT_FS
> > > > > +choice
> > > > > +	prompt "Filesystem type"
> > > > > +	default BOOTCOUNT_EXT
> > > > > +
> > > > > +config BOOTCOUNT_EXT
> > > > > +	bool "Boot counter on EXT filesystem"
> > > > > +	depends on FS_EXT4
> > > > > +	select EXT4_WRITE
> > > > > +	help
> > > > > +	  Add support for maintaing boot counter in a file on EXT filesystem"
> > > > > +
> > > > > +config BOOTCOUNT_FAT
> > > > > +	bool "Boot counter on FAT filesystem"
> > > > > +	depends on FS_FAT
> > > > > +	select FAT_WRITE
> > > > > +	help
> > > > > +	  Add support for maintaing boot counter in a file on FAT filesystem"
> > > > > +
> > 
> > Seems like I missed a typo here as well:
> > 
> > s/maintaing/maintaining/ ? At least that's what we have in
> > doc/README.bootcount
> > 
> > > > > +endchoice
> > > > > +endif
> > > > > +
> > > > Since we now support FS_ANY, do we really need this choice at all?
> > > > 
> > > > Alternatively, should it **really** be a choice and not just a bunch of
> > > > configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
> > > > instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
> > > > without issue?
> > > > 
> > > > Cheers,
> > > > Quentin
> > > 
> > > Well, I think I kind of get the point but I am still a bit confused.
> > > Do you mean that basically the configuration should be done the other way
> > > around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
> > > EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
> > > If yes, what is the advantage of this approach?
> > > 
> > 
> > I'm suggesting:
> > 
> > """
> > config BOOTCOUNT_FS
> > 	bool "Boot counter on a filesystem"
> >  	help
> > 
> > config BOOTCOUNT_EXT
> > 	bool "Boot counter on EXT filesystem"
> >         default y
> > 	depends on BOOTCOUNT_FS
> > 	depends on FS_EXT4
> > 	select EXT4_WRITE
> > 	help
> > 	  Add support for maintaing boot counter in a file on EXT filesystem"
> > 
> > config BOOTCOUNT_FAT
> > 	bool "Boot counter on FAT filesystem"
> > 	depends on BOOTCOUNT_FS
> > 	depends on FS_FAT
> > 	select FAT_WRITE
> > 	help
> > 	  Add support for maintaing boot counter in a file on FAT filesystem"
> > """
> > 
> > This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT are
> > both selected, the user would then be free to decide if the same partition
> > on two different devices but for the same purpose can be either ext2/3/4 or
> > FAT, without recompiling U-Boot just for that.
> > 
> > However, it would now be possible to have BOOTCOUNT_FS=y but neither
> > BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT isn't
> > defined).
> > 
> > Finally, the other option was just to NOT have BOOTCOUNT_FAT or
> > BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and EXT4_WRITE/FAT_WRITE
> > themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code. This
> > is less user-friendly though.
> 
> I was thinking that with FS_ANY, we don't need a symbol per filesystem
> type but can just handle it in the help text with something like "Please
> ensure that you have enabled write support for the filesystem that you
> will be used by the partition that you configure this feature for."
> 

Hi Tom,

I see that both you and Quentin are leaning towards this solution which is
totally fine by me. As I said before, the user already needs to specify the
device interface and the partition to be used so they can also be
responsible to enable the appropriate filesystem write functionality. Indeed
if more filesystems are added, it would be quite ugly to have multiple
symbols just to choose a config.

Also, if the user forgets to enable the appropriate *_WRITE config, it will
trigger an error message in U-Boot that this operation is not supported
which will be pretty obvious what needs to be done.

So I can go with a v3, following your proposals.

Cheers,
Vasilis
> -- 
> Tom

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Quentin Schulz]

Hi Vasileios,

On 6/11/24 6:20 PM, Vasileios Amoiridis wrote:
> On Tue, Jun 11, 2024 at 10:06:59AM -0600, Tom Rini wrote:
>> On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
>>> Hi Vasileios,
>>>
>>> On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
>>>> On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
>>>>> Hi Vasileios,
>>>>>
>>>>> On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
>>>>>> Add support to save boot count variable in a file in a FAT filesystem.
>>>>>>
>>>>>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
>>>>>> ---
>>>>>>     doc/README.bootcount                          | 12 ++---
>>>>>>     drivers/bootcount/Kconfig                     | 53 +++++++++++++------
>>>>>>     drivers/bootcount/Makefile                    |  2 +-
>>>>>>     .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
>>>>>>     4 files changed, 50 insertions(+), 29 deletions(-)
>>>>>>     rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
>>>>>>
>>>>>> diff --git a/doc/README.bootcount b/doc/README.bootcount
>>>>>> index f6c5f82f..0f4ffb68 100644
>>>>>> --- a/doc/README.bootcount
>>>>>> +++ b/doc/README.bootcount
>>>>>> @@ -23,15 +23,15 @@ It is the responsibility of some application code
>>>>> (typically a Linux
>>>>>>     application) to reset the variable "bootcount" to 0 when the system
>>>>> booted
>>>>>>     successfully, thus allowing for more boot cycles.
>>>>>>
>>>>>> -CONFIG_BOOTCOUNT_EXT
>>>>>> +CONFIG_BOOTCOUNT_FS
>>>>>>     --------------------
>>>>>>
>>>>>> -This adds support for maintaining boot count in a file on an EXT
>>>>> filesystem.
>>>>>> -The file to use is defined by:
>>>>>> +This adds support for maintaining boot count in a file on a filesystem.
>>>>>> +Supported filesystems are FAT and EXT. The file to use is defined by:
>>>>>>
>>>>>> -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
>>>>>> -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
>>>>>> -CONFIG_SYS_BOOTCOUNT_EXT_NAME
>>>>>> +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
>>>>>> +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
>>>>>> +CONFIG_SYS_BOOTCOUNT_FS_NAME
>>>>>>
>>>>>>     The format of the file is:
>>>>>>
>>>>>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>>>>>> index 3c56253b..d3679eb5 100644
>>>>>> --- a/drivers/bootcount/Kconfig
>>>>>> +++ b/drivers/bootcount/Kconfig
>>>>>> @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
>>>>>>     	    Set to the address where the bootcount and bootcount magic
>>>>>>     	    will be stored.
>>>>>>
>>>>>> -config BOOTCOUNT_EXT
>>>>>> -	bool "Boot counter on EXT filesystem"
>>>>>> -	depends on FS_EXT4
>>>>>> -	select EXT4_WRITE
>>>>>> +config BOOTCOUNT_FS
>>>>>> +	bool "Boot counter on a filesystem"
>>>>>> +	depends on FS_EXT4 || FS_FAT
>>>>> Do we really need this 'depends on' here? Especially if we have a choice
>>>>> below...
>>>>>
>>>> Well, probably this is redundant indeed.
>>>>
>>>>>>     	help
>>>>>>     	  Add support for maintaining boot count in a file on an EXT
>>>>> The help text is still mentioning EXT here.
>>>>>
>>>>
>>>> Ahh, I missed that.
>>>>
>>>>> I would recommend removing it, or listing the supported filesystems at the
>>>>> moment. While I assume you tested with FAT, I assume that with FS_ANY, any
>>>>> FS should be supported?
>>>>>
>>>>
>>>> Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
>>>> implementation of the filesystem handling code in U-Boot, if the fs supports
>>>> a write a function, then it should work. But I cannot test for other
>>>> filesystems apart from FAT and EXT4 so I think it's better to limit the
>>>> option to these two.
>>>>
>>>
>>> I guess we can let people figure things out themselves and add new options
>>> for when they have tested them, no strong opinion here.
>>>
>>>>>>     	  filesystem.
>>>>>> @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
>>>>>>     	  This option enables packing boot count magic value and boot count
>>>>>>     	  into single word (32 bits).
>>>>>>
>>>>>> -config SYS_BOOTCOUNT_EXT_INTERFACE
>>>>>> -	string "Interface on which to find boot counter EXT filesystem"
>>>>>> +if BOOTCOUNT_FS
>>>>>> +choice
>>>>>> +	prompt "Filesystem type"
>>>>>> +	default BOOTCOUNT_EXT
>>>>>> +
>>>>>> +config BOOTCOUNT_EXT
>>>>>> +	bool "Boot counter on EXT filesystem"
>>>>>> +	depends on FS_EXT4
>>>>>> +	select EXT4_WRITE
>>>>>> +	help
>>>>>> +	  Add support for maintaing boot counter in a file on EXT filesystem"
>>>>>> +
>>>>>> +config BOOTCOUNT_FAT
>>>>>> +	bool "Boot counter on FAT filesystem"
>>>>>> +	depends on FS_FAT
>>>>>> +	select FAT_WRITE
>>>>>> +	help
>>>>>> +	  Add support for maintaing boot counter in a file on FAT filesystem"
>>>>>> +
>>>
>>> Seems like I missed a typo here as well:
>>>
>>> s/maintaing/maintaining/ ? At least that's what we have in
>>> doc/README.bootcount
>>>
>>>>>> +endchoice
>>>>>> +endif
>>>>>> +
>>>>> Since we now support FS_ANY, do we really need this choice at all?
>>>>>
>>>>> Alternatively, should it **really** be a choice and not just a bunch of
>>>>> configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
>>>>> instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
>>>>> without issue?
>>>>>
>>>>> Cheers,
>>>>> Quentin
>>>>
>>>> Well, I think I kind of get the point but I am still a bit confused.
>>>> Do you mean that basically the configuration should be done the other way
>>>> around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
>>>> EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
>>>> If yes, what is the advantage of this approach?
>>>>
>>>
>>> I'm suggesting:
>>>
>>> """
>>> config BOOTCOUNT_FS
>>> 	bool "Boot counter on a filesystem"
>>>   	help
>>>
>>> config BOOTCOUNT_EXT
>>> 	bool "Boot counter on EXT filesystem"
>>>          default y
>>> 	depends on BOOTCOUNT_FS
>>> 	depends on FS_EXT4
>>> 	select EXT4_WRITE
>>> 	help
>>> 	  Add support for maintaing boot counter in a file on EXT filesystem"
>>>
>>> config BOOTCOUNT_FAT
>>> 	bool "Boot counter on FAT filesystem"
>>> 	depends on BOOTCOUNT_FS
>>> 	depends on FS_FAT
>>> 	select FAT_WRITE
>>> 	help
>>> 	  Add support for maintaing boot counter in a file on FAT filesystem"
>>> """
>>>
>>> This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT are
>>> both selected, the user would then be free to decide if the same partition
>>> on two different devices but for the same purpose can be either ext2/3/4 or
>>> FAT, without recompiling U-Boot just for that.
>>>
>>> However, it would now be possible to have BOOTCOUNT_FS=y but neither
>>> BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT isn't
>>> defined).
>>>
>>> Finally, the other option was just to NOT have BOOTCOUNT_FAT or
>>> BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and EXT4_WRITE/FAT_WRITE
>>> themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code. This
>>> is less user-friendly though.
>>
>> I was thinking that with FS_ANY, we don't need a symbol per filesystem
>> type but can just handle it in the help text with something like "Please
>> ensure that you have enabled write support for the filesystem that you
>> will be used by the partition that you configure this feature for."
>>
> 
> Hi Tom,
> 
> I see that both you and Quentin are leaning towards this solution which is
> totally fine by me. As I said before, the user already needs to specify the
> device interface and the partition to be used so they can also be
> responsible to enable the appropriate filesystem write functionality. Indeed
> if more filesystems are added, it would be quite ugly to have multiple
> symbols just to choose a config.
> 

I wouldn't be surprised if some time in the future we make the device 
interface and partition configurable through a U-Boot environment.

Also, there's no need for U-Boot to know the fs to write to since it can 
be auto-detected, so adding an artificial hurdle isn't necessarily the 
best design choice.

Basically, I can have two products based on the same HW design. I have 
one with an EXT4 partition, and another for FAT partition, they both are 
on the same storage medium (one on one product, the other on the other 
product) in the same partition number. If we had an exclusive choice, 
then we would have to have two different U-Boot for essentially the same 
HW just based on the software design decisions. We cannot always avoid 
this, but I think we can here, so I think this would be pretty nice to 
have :)

> Also, if the user forgets to enable the appropriate *_WRITE config, it will
> trigger an error message in U-Boot that this operation is not supported
> which will be pretty obvious what needs to be done.
> 

OK, that's good news. I hope the message is explicit enough that users 
know what to do with the info. It's better than crashing U-Boot, which 
sometimes happen when in SPL or U-Boot proper pre-reloc for mysterious 
reasons and make things very frustrating to debug :)

> So I can go with a v3, following your proposals.
> 

Seems to me like this may be the way to go yes.

Cheers,
Quentin

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

On Tue, Jun 11, 2024 at 06:29:34PM +0200, Quentin Schulz wrote:
> Hi Vasileios,
> 
> On 6/11/24 6:20 PM, Vasileios Amoiridis wrote:
> > On Tue, Jun 11, 2024 at 10:06:59AM -0600, Tom Rini wrote:
> > > On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
> > > > Hi Vasileios,
> > > > 
> > > > On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> > > > > On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> > > > > > Hi Vasileios,
> > > > > > 
> > > > > > On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
> > > > > > > Add support to save boot count variable in a file in a FAT filesystem.
> > > > > > > 
> > > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > > > > ---
> > > > > > >     doc/README.bootcount                          | 12 ++---
> > > > > > >     drivers/bootcount/Kconfig                     | 53 +++++++++++++------
> > > > > > >     drivers/bootcount/Makefile                    |  2 +-
> > > > > > >     .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
> > > > > > >     4 files changed, 50 insertions(+), 29 deletions(-)
> > > > > > >     rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
> > > > > > > 
> > > > > > > diff --git a/doc/README.bootcount b/doc/README.bootcount
> > > > > > > index f6c5f82f..0f4ffb68 100644
> > > > > > > --- a/doc/README.bootcount
> > > > > > > +++ b/doc/README.bootcount
> > > > > > > @@ -23,15 +23,15 @@ It is the responsibility of some application code
> > > > > > (typically a Linux
> > > > > > >     application) to reset the variable "bootcount" to 0 when the system
> > > > > > booted
> > > > > > >     successfully, thus allowing for more boot cycles.
> > > > > > > 
> > > > > > > -CONFIG_BOOTCOUNT_EXT
> > > > > > > +CONFIG_BOOTCOUNT_FS
> > > > > > >     --------------------
> > > > > > > 
> > > > > > > -This adds support for maintaining boot count in a file on an EXT
> > > > > > filesystem.
> > > > > > > -The file to use is defined by:
> > > > > > > +This adds support for maintaining boot count in a file on a filesystem.
> > > > > > > +Supported filesystems are FAT and EXT. The file to use is defined by:
> > > > > > > 
> > > > > > > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
> > > > > > > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
> > > > > > > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
> > > > > > > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
> > > > > > > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
> > > > > > > +CONFIG_SYS_BOOTCOUNT_FS_NAME
> > > > > > > 
> > > > > > >     The format of the file is:
> > > > > > > 
> > > > > > > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > > > > > > index 3c56253b..d3679eb5 100644
> > > > > > > --- a/drivers/bootcount/Kconfig
> > > > > > > +++ b/drivers/bootcount/Kconfig
> > > > > > > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
> > > > > > >     	    Set to the address where the bootcount and bootcount magic
> > > > > > >     	    will be stored.
> > > > > > > 
> > > > > > > -config BOOTCOUNT_EXT
> > > > > > > -	bool "Boot counter on EXT filesystem"
> > > > > > > -	depends on FS_EXT4
> > > > > > > -	select EXT4_WRITE
> > > > > > > +config BOOTCOUNT_FS
> > > > > > > +	bool "Boot counter on a filesystem"
> > > > > > > +	depends on FS_EXT4 || FS_FAT
> > > > > > Do we really need this 'depends on' here? Especially if we have a choice
> > > > > > below...
> > > > > > 
> > > > > Well, probably this is redundant indeed.
> > > > > 
> > > > > > >     	help
> > > > > > >     	  Add support for maintaining boot count in a file on an EXT
> > > > > > The help text is still mentioning EXT here.
> > > > > > 
> > > > > 
> > > > > Ahh, I missed that.
> > > > > 
> > > > > > I would recommend removing it, or listing the supported filesystems at the
> > > > > > moment. While I assume you tested with FAT, I assume that with FS_ANY, any
> > > > > > FS should be supported?
> > > > > > 
> > > > > 
> > > > > Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
> > > > > implementation of the filesystem handling code in U-Boot, if the fs supports
> > > > > a write a function, then it should work. But I cannot test for other
> > > > > filesystems apart from FAT and EXT4 so I think it's better to limit the
> > > > > option to these two.
> > > > > 
> > > > 
> > > > I guess we can let people figure things out themselves and add new options
> > > > for when they have tested them, no strong opinion here.
> > > > 
> > > > > > >     	  filesystem.
> > > > > > > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
> > > > > > >     	  This option enables packing boot count magic value and boot count
> > > > > > >     	  into single word (32 bits).
> > > > > > > 
> > > > > > > -config SYS_BOOTCOUNT_EXT_INTERFACE
> > > > > > > -	string "Interface on which to find boot counter EXT filesystem"
> > > > > > > +if BOOTCOUNT_FS
> > > > > > > +choice
> > > > > > > +	prompt "Filesystem type"
> > > > > > > +	default BOOTCOUNT_EXT
> > > > > > > +
> > > > > > > +config BOOTCOUNT_EXT
> > > > > > > +	bool "Boot counter on EXT filesystem"
> > > > > > > +	depends on FS_EXT4
> > > > > > > +	select EXT4_WRITE
> > > > > > > +	help
> > > > > > > +	  Add support for maintaing boot counter in a file on EXT filesystem"
> > > > > > > +
> > > > > > > +config BOOTCOUNT_FAT
> > > > > > > +	bool "Boot counter on FAT filesystem"
> > > > > > > +	depends on FS_FAT
> > > > > > > +	select FAT_WRITE
> > > > > > > +	help
> > > > > > > +	  Add support for maintaing boot counter in a file on FAT filesystem"
> > > > > > > +
> > > > 
> > > > Seems like I missed a typo here as well:
> > > > 
> > > > s/maintaing/maintaining/ ? At least that's what we have in
> > > > doc/README.bootcount
> > > > 
> > > > > > > +endchoice
> > > > > > > +endif
> > > > > > > +
> > > > > > Since we now support FS_ANY, do we really need this choice at all?
> > > > > > 
> > > > > > Alternatively, should it **really** be a choice and not just a bunch of
> > > > > > configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
> > > > > > instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
> > > > > > without issue?
> > > > > > 
> > > > > > Cheers,
> > > > > > Quentin
> > > > > 
> > > > > Well, I think I kind of get the point but I am still a bit confused.
> > > > > Do you mean that basically the configuration should be done the other way
> > > > > around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
> > > > > EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
> > > > > If yes, what is the advantage of this approach?
> > > > > 
> > > > 
> > > > I'm suggesting:
> > > > 
> > > > """
> > > > config BOOTCOUNT_FS
> > > > 	bool "Boot counter on a filesystem"
> > > >   	help
> > > > 
> > > > config BOOTCOUNT_EXT
> > > > 	bool "Boot counter on EXT filesystem"
> > > >          default y
> > > > 	depends on BOOTCOUNT_FS
> > > > 	depends on FS_EXT4
> > > > 	select EXT4_WRITE
> > > > 	help
> > > > 	  Add support for maintaing boot counter in a file on EXT filesystem"
> > > > 
> > > > config BOOTCOUNT_FAT
> > > > 	bool "Boot counter on FAT filesystem"
> > > > 	depends on BOOTCOUNT_FS
> > > > 	depends on FS_FAT
> > > > 	select FAT_WRITE
> > > > 	help
> > > > 	  Add support for maintaing boot counter in a file on FAT filesystem"
> > > > """
> > > > 
> > > > This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT are
> > > > both selected, the user would then be free to decide if the same partition
> > > > on two different devices but for the same purpose can be either ext2/3/4 or
> > > > FAT, without recompiling U-Boot just for that.
> > > > 
> > > > However, it would now be possible to have BOOTCOUNT_FS=y but neither
> > > > BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT isn't
> > > > defined).
> > > > 
> > > > Finally, the other option was just to NOT have BOOTCOUNT_FAT or
> > > > BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and EXT4_WRITE/FAT_WRITE
> > > > themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code. This
> > > > is less user-friendly though.
> > > 
> > > I was thinking that with FS_ANY, we don't need a symbol per filesystem
> > > type but can just handle it in the help text with something like "Please
> > > ensure that you have enabled write support for the filesystem that you
> > > will be used by the partition that you configure this feature for."
> > > 
> > 
> > Hi Tom,
> > 
> > I see that both you and Quentin are leaning towards this solution which is
> > totally fine by me. As I said before, the user already needs to specify the
> > device interface and the partition to be used so they can also be
> > responsible to enable the appropriate filesystem write functionality. Indeed
> > if more filesystems are added, it would be quite ugly to have multiple
> > symbols just to choose a config.
> > 
> 
> I wouldn't be surprised if some time in the future we make the device
> interface and partition configurable through a U-Boot environment.
> 
> Also, there's no need for U-Boot to know the fs to write to since it can be
> auto-detected, so adding an artificial hurdle isn't necessarily the best
> design choice.
> 

True.

> Basically, I can have two products based on the same HW design. I have one
> with an EXT4 partition, and another for FAT partition, they both are on the
> same storage medium (one on one product, the other on the other product) in
> the same partition number. If we had an exclusive choice, then we would have
> to have two different U-Boot for essentially the same HW just based on the
> software design decisions. We cannot always avoid this, but I think we can
> here, so I think this would be pretty nice to have :)
> 

Well, that's a good example, so totally understandable, thanks!!!

> > Also, if the user forgets to enable the appropriate *_WRITE config, it will
> > trigger an error message in U-Boot that this operation is not supported
> > which will be pretty obvious what needs to be done.
> > 
> 
> OK, that's good news. I hope the message is explicit enough that users know
> what to do with the info. It's better than crashing U-Boot, which sometimes
> happen when in SPL or U-Boot proper pre-reloc for mysterious reasons and
> make things very frustrating to debug :)
> 
> > So I can go with a v3, following your proposals.
> > 
> 
> Seems to me like this may be the way to go yes.
> 
> Cheers,
> Quentin

Cheers,
Vasilis

Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Vasileios Amoiridis]

On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
> Hi Vasileios,
> 
> On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> > On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> > > Hi Vasileios,
> > > 
> > > On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
> > > > Add support to save boot count variable in a file in a FAT filesystem.
> > > > 
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > ---
> > > >    doc/README.bootcount                          | 12 ++---
> > > >    drivers/bootcount/Kconfig                     | 53 +++++++++++++------
> > > >    drivers/bootcount/Makefile                    |  2 +-
> > > >    .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
> > > >    4 files changed, 50 insertions(+), 29 deletions(-)
> > > >    rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
> > > > 
> > > > diff --git a/doc/README.bootcount b/doc/README.bootcount
> > > > index f6c5f82f..0f4ffb68 100644
> > > > --- a/doc/README.bootcount
> > > > +++ b/doc/README.bootcount
> > > > @@ -23,15 +23,15 @@ It is the responsibility of some application code
> > > (typically a Linux
> > > >    application) to reset the variable "bootcount" to 0 when the system
> > > booted
> > > >    successfully, thus allowing for more boot cycles.
> > > > 
> > > > -CONFIG_BOOTCOUNT_EXT
> > > > +CONFIG_BOOTCOUNT_FS
> > > >    --------------------
> > > > 
> > > > -This adds support for maintaining boot count in a file on an EXT
> > > filesystem.
> > > > -The file to use is defined by:
> > > > +This adds support for maintaining boot count in a file on a filesystem.
> > > > +Supported filesystems are FAT and EXT. The file to use is defined by:
> > > > 
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
> > > > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
> > > > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
> > > > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
> > > > +CONFIG_SYS_BOOTCOUNT_FS_NAME
> > > > 
> > > >    The format of the file is:
> > > > 
> > > > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > > > index 3c56253b..d3679eb5 100644
> > > > --- a/drivers/bootcount/Kconfig
> > > > +++ b/drivers/bootcount/Kconfig
> > > > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
> > > >    	    Set to the address where the bootcount and bootcount magic
> > > >    	    will be stored.
> > > > 
> > > > -config BOOTCOUNT_EXT
> > > > -	bool "Boot counter on EXT filesystem"
> > > > -	depends on FS_EXT4
> > > > -	select EXT4_WRITE
> > > > +config BOOTCOUNT_FS
> > > > +	bool "Boot counter on a filesystem"
> > > > +	depends on FS_EXT4 || FS_FAT
> > > Do we really need this 'depends on' here? Especially if we have a choice
> > > below...
> > > 
> > Well, probably this is redundant indeed.
> > 
> > > >    	help
> > > >    	  Add support for maintaining boot count in a file on an EXT
> > > The help text is still mentioning EXT here.
> > > 
> > 
> > Ahh, I missed that.
> > 
> > > I would recommend removing it, or listing the supported filesystems at the
> > > moment. While I assume you tested with FAT, I assume that with FS_ANY, any
> > > FS should be supported?
> > > 
> > 
> > Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
> > implementation of the filesystem handling code in U-Boot, if the fs supports
> > a write a function, then it should work. But I cannot test for other
> > filesystems apart from FAT and EXT4 so I think it's better to limit the
> > option to these two.
> > 
> 
> I guess we can let people figure things out themselves and add new options
> for when they have tested them, no strong opinion here.
> 
> > > >    	  filesystem.
> > > > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
> > > >    	  This option enables packing boot count magic value and boot count
> > > >    	  into single word (32 bits).
> > > > 
> > > > -config SYS_BOOTCOUNT_EXT_INTERFACE
> > > > -	string "Interface on which to find boot counter EXT filesystem"
> > > > +if BOOTCOUNT_FS
> > > > +choice
> > > > +	prompt "Filesystem type"
> > > > +	default BOOTCOUNT_EXT
> > > > +
> > > > +config BOOTCOUNT_EXT
> > > > +	bool "Boot counter on EXT filesystem"
> > > > +	depends on FS_EXT4
> > > > +	select EXT4_WRITE
> > > > +	help
> > > > +	  Add support for maintaing boot counter in a file on EXT filesystem"
> > > > +
> > > > +config BOOTCOUNT_FAT
> > > > +	bool "Boot counter on FAT filesystem"
> > > > +	depends on FS_FAT
> > > > +	select FAT_WRITE
> > > > +	help
> > > > +	  Add support for maintaing boot counter in a file on FAT filesystem"
> > > > +
> 
> Seems like I missed a typo here as well:
> 
> s/maintaing/maintaining/ ? At least that's what we have in
> doc/README.bootcount
> 
> > > > +endchoice
> > > > +endif
> > > > +
> > > Since we now support FS_ANY, do we really need this choice at all?
> > > 
> > > Alternatively, should it **really** be a choice and not just a bunch of
> > > configs that depends on BOOTCOUNT_FS + whatever's needed to write on that FS
> > > instead? I think we could have both BOOTCOUNT_EXT and BOOTCOUNT_FAT set
> > > without issue?
> > > 
> > > Cheers,
> > > Quentin
> > 
> > Well, I think I kind of get the point but I am still a bit confused.
> > Do you mean that basically the configuration should be done the other way
> > around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
> > EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
> > If yes, what is the advantage of this approach?
> > 
> 
> I'm suggesting:
> 
> """
> config BOOTCOUNT_FS
> 	bool "Boot counter on a filesystem"
>  	help
> 
> config BOOTCOUNT_EXT
> 	bool "Boot counter on EXT filesystem"
>         default y
> 	depends on BOOTCOUNT_FS
> 	depends on FS_EXT4
> 	select EXT4_WRITE
> 	help
> 	  Add support for maintaing boot counter in a file on EXT filesystem"
> 
> config BOOTCOUNT_FAT
> 	bool "Boot counter on FAT filesystem"
> 	depends on BOOTCOUNT_FS
> 	depends on FS_FAT
> 	select FAT_WRITE
> 	help
> 	  Add support for maintaing boot counter in a file on FAT filesystem"
> """
> 
> This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT are
> both selected, the user would then be free to decide if the same partition
> on two different devices but for the same purpose can be either ext2/3/4 or
> FAT, without recompiling U-Boot just for that.
> 
> However, it would now be possible to have BOOTCOUNT_FS=y but neither
> BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT isn't
> defined).
> 

But why having both *_FAT and *_EXT selected at the same time? You have to
specifically mention the device interface and the number of partition to use
from that interface. Also, the bootcount is kept in just one place, so having
enabled both wouldn't it make it incosistent? 

> Finally, the other option was just to NOT have BOOTCOUNT_FAT or
> BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and EXT4_WRITE/FAT_WRITE
> themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code. This
> is less user-friendly though.
> 

This is indeed less user-friendly but on the other hand, the device interface
and the partition number have to be set by the user. So leaving also this
choice to the user shouldn't be that big of a problem, right?

> Cheers,
> Quentin