[U-Boot,8/8] Migrate generic bootcount to Kconfig

Message ID 1518350813-3418-9-git-send-email-alex.kiernan@gmail.com
State Superseded
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot,1/8] Merge CONFIG_BOOTCOUNT and CONFIG_BOOTCOUNT_LIMIT
Related show

Commit Message

Alex Kiernan Feb. 11, 2018, 12:06 p.m.
Make generate boot counter selected in the same way as other boot count
drivers

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

 drivers/bootcount/Kconfig  | 11 +++++++++++
 drivers/bootcount/Makefile |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Lukasz Majewski Feb. 11, 2018, 7:36 p.m. | #1
Hi Alex,

> Make generate boot counter selected in the same way as other boot
> count drivers
> 
> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> ---
> 
>  drivers/bootcount/Kconfig  | 11 +++++++++++
>  drivers/bootcount/Makefile |  2 +-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> index e0d1fc2..9fde2f2 100644
> --- a/drivers/bootcount/Kconfig
> +++ b/drivers/bootcount/Kconfig
> @@ -14,6 +14,16 @@ choice
>  	prompt "Boot count device"
>  	default BOOTCOUNT_AM33XX if AM33XX || SOC_DA8XX
>  	default BOOTCOUNT_AT91 if AT91SAM9XE
> +	default BOOTCOUNT_GENERIC
> +
> +config BOOTCOUNT_GENERIC
> +	bool "Generic default boot counter"
> +	help
> +	  Generic bootcount stored at SYS_BOOTCOUNT_ADDR.
> +
> +	  SYS_BOOTCOUNT_ADDR:
> +	    Set to the address where the bootcount and bootcount
> magic
> +	    will be stored.
>  
>  config BOOTCOUNT_EXT
>  	bool "Boot counter on EXT filesystem"
> @@ -64,6 +74,7 @@ endchoice
>  
>  config SYS_BOOTCOUNT_SINGLEWORD
>  	bool "Use single word to pack boot count and magic value"
> +	depends on BOOTCOUNT_GENERIC
>  	help
>  	  This option enables packing boot count magic value and
> boot count into single word (32 bits).
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index a3658c1..3e1ae8c 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -2,7 +2,7 @@
>  # SPDX-License-Identifier:	GPL-2.0+
>  #
>  
> -obj-y				+= bootcount.o
> +obj-$(CONFIG_BOOTCOUNT_GENERIC)	+= bootcount.o
>  obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
>  obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
>  obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o

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


I had to put attached patch (one liner) to make it working on my setup
(this allows re-using the SYS_BOOTCOUNT_ADDR on non EXT setup).

Could you squash this patch to your work and send v2?

Thanks in advance,

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
From 7d2ee7df8c5b8bfaa6e7d56d5183e45ab00f8692 Mon Sep 17 00:00:00 2001
From: Lukasz Majewski <lukma@denx.de>
Date: Sun, 11 Feb 2018 20:28:44 +0100
Subject: [PATCH] bootcount: Kconfig: Enable CONFIG_SYS_BOOTCOUNT_ADDR to be
 available not only for BOOTCOUNT_EXT

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/bootcount/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index 9fde2f2a66..bb869c94fb 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -105,7 +105,7 @@ config SYS_BOOTCOUNT_EXT_NAME
 config SYS_BOOTCOUNT_ADDR
 	hex "RAM address used for reading and writing the boot counter"
 	default 0x7000A000
-	depends on BOOTCOUNT_EXT
+	depends on BOOTCOUNT_EXT || BOOTCOUNT_I2C || BOOTCOUNT_GENERIC
 	help
 	  Set the address used for reading and writing the boot counter.
Alex Kiernan Feb. 11, 2018, 9:04 p.m. | #2
On Sun, Feb 11, 2018 at 7:36 PM, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Alex,
>
>> Make generate boot counter selected in the same way as other boot
>> count drivers
>>
>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
>> ---
>>
>>  drivers/bootcount/Kconfig  | 11 +++++++++++
>>  drivers/bootcount/Makefile |  2 +-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>> index e0d1fc2..9fde2f2 100644
>> --- a/drivers/bootcount/Kconfig
>> +++ b/drivers/bootcount/Kconfig
>> @@ -14,6 +14,16 @@ choice
>>       prompt "Boot count device"
>>       default BOOTCOUNT_AM33XX if AM33XX || SOC_DA8XX
>>       default BOOTCOUNT_AT91 if AT91SAM9XE
>> +     default BOOTCOUNT_GENERIC
>> +
>> +config BOOTCOUNT_GENERIC
>> +     bool "Generic default boot counter"
>> +     help
>> +       Generic bootcount stored at SYS_BOOTCOUNT_ADDR.
>> +
>> +       SYS_BOOTCOUNT_ADDR:
>> +         Set to the address where the bootcount and bootcount
>> magic
>> +         will be stored.
>>
>>  config BOOTCOUNT_EXT
>>       bool "Boot counter on EXT filesystem"
>> @@ -64,6 +74,7 @@ endchoice
>>
>>  config SYS_BOOTCOUNT_SINGLEWORD
>>       bool "Use single word to pack boot count and magic value"
>> +     depends on BOOTCOUNT_GENERIC
>>       help
>>         This option enables packing boot count magic value and
>> boot count into single word (32 bits).
>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>> index a3658c1..3e1ae8c 100644
>> --- a/drivers/bootcount/Makefile
>> +++ b/drivers/bootcount/Makefile
>> @@ -2,7 +2,7 @@
>>  # SPDX-License-Identifier:   GPL-2.0+
>>  #
>>
>> -obj-y                                += bootcount.o
>> +obj-$(CONFIG_BOOTCOUNT_GENERIC)      += bootcount.o
>>  obj-$(CONFIG_BOOTCOUNT_AT91) += bootcount_at91.o
>>  obj-$(CONFIG_BOOTCOUNT_AM33XX)       += bootcount_davinci.o
>>  obj-$(CONFIG_BOOTCOUNT_RAM)  += bootcount_ram.o
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
>
>
> I had to put attached patch (one liner) to make it working on my setup
> (this allows re-using the SYS_BOOTCOUNT_ADDR on non EXT setup).
>
> Could you squash this patch to your work and send v2?
>

I'm not really sure what the right thing to do with SYS_BOOTCOUNT_ADDR is...

The default is only right for BOOTCOUNT_EXT (and then only on a
specific board?) and elsewhere it's mostly set in board configs. In my
case I actually want it to be defined based on a other bits of memory
map . Maybe it's been overloaded too much and really wants to be a
variable per driver?

That said, squashing in that change doesn't obviously break anything
for me, and is probably a step in the right direction.

I'll see what Travis thinks.
Lukasz Majewski Feb. 11, 2018, 9:44 p.m. | #3
On Sun, 11 Feb 2018 21:04:46 +0000
Alex Kiernan <alex.kiernan@gmail.com> wrote:

> On Sun, Feb 11, 2018 at 7:36 PM, Lukasz Majewski <lukma@denx.de>
> wrote:
> > Hi Alex,
> >  
> >> Make generate boot counter selected in the same way as other boot
> >> count drivers
> >>
> >> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> >> ---
> >>
> >>  drivers/bootcount/Kconfig  | 11 +++++++++++
> >>  drivers/bootcount/Makefile |  2 +-
> >>  2 files changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> >> index e0d1fc2..9fde2f2 100644
> >> --- a/drivers/bootcount/Kconfig
> >> +++ b/drivers/bootcount/Kconfig
> >> @@ -14,6 +14,16 @@ choice
> >>       prompt "Boot count device"
> >>       default BOOTCOUNT_AM33XX if AM33XX || SOC_DA8XX
> >>       default BOOTCOUNT_AT91 if AT91SAM9XE
> >> +     default BOOTCOUNT_GENERIC
> >> +
> >> +config BOOTCOUNT_GENERIC
> >> +     bool "Generic default boot counter"
> >> +     help
> >> +       Generic bootcount stored at SYS_BOOTCOUNT_ADDR.
> >> +
> >> +       SYS_BOOTCOUNT_ADDR:
> >> +         Set to the address where the bootcount and bootcount
> >> magic
> >> +         will be stored.
> >>
> >>  config BOOTCOUNT_EXT
> >>       bool "Boot counter on EXT filesystem"
> >> @@ -64,6 +74,7 @@ endchoice
> >>
> >>  config SYS_BOOTCOUNT_SINGLEWORD
> >>       bool "Use single word to pack boot count and magic value"
> >> +     depends on BOOTCOUNT_GENERIC
> >>       help
> >>         This option enables packing boot count magic value and
> >> boot count into single word (32 bits).
> >> diff --git a/drivers/bootcount/Makefile
> >> b/drivers/bootcount/Makefile index a3658c1..3e1ae8c 100644
> >> --- a/drivers/bootcount/Makefile
> >> +++ b/drivers/bootcount/Makefile
> >> @@ -2,7 +2,7 @@
> >>  # SPDX-License-Identifier:   GPL-2.0+
> >>  #
> >>
> >> -obj-y                                += bootcount.o
> >> +obj-$(CONFIG_BOOTCOUNT_GENERIC)      += bootcount.o
> >>  obj-$(CONFIG_BOOTCOUNT_AT91) += bootcount_at91.o
> >>  obj-$(CONFIG_BOOTCOUNT_AM33XX)       += bootcount_davinci.o
> >>  obj-$(CONFIG_BOOTCOUNT_RAM)  += bootcount_ram.o  
> >
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> >
> >
> > I had to put attached patch (one liner) to make it working on my
> > setup (this allows re-using the SYS_BOOTCOUNT_ADDR on non EXT
> > setup).
> >
> > Could you squash this patch to your work and send v2?
> >  
> 
> I'm not really sure what the right thing to do with
> SYS_BOOTCOUNT_ADDR is...
> 
> The default is only right for BOOTCOUNT_EXT (and then only on a
> specific board?) and elsewhere it's mostly set in board configs. In my
> case I actually want it to be defined based on a other bits of memory
> map . Maybe it's been overloaded too much and really wants to be a
> variable per driver?

It is defined in a few places though:

git grep -n "SYS_BOOTCOUNT_ADDR" [1]

However, for my builds I did not observed the build breaks (build
definately breaks when this value is "") - but in my patch I do also
use BOOTCOUNT, which is not defined in [1] boards.

Hence, (when BOOTCOUNT is not defined) boards [1] use
SYS_BOOTCOUNT_ADDR from their ./include/configs/<board>.h.

When, I do define BOOTCOUNT on my board, then I _must_ also define
correct SYS_BOOTCOUNT_ADDR in my ./configs/<board>_defconfig.

Please look into following patchset:
http://patchwork.ozlabs.org/cover/871655/


> 
> That said, squashing in that change doesn't obviously break anything
> for me, and is probably a step in the right direction.
> 
> I'll see what Travis thinks.
> 

We will probably receive build breaks...


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
Alex Kiernan Feb. 12, 2018, 5:55 a.m. | #4
On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de> wrote:
> On Sun, 11 Feb 2018 21:04:46 +0000
> Alex Kiernan <alex.kiernan@gmail.com> wrote:
>
>>
>> That said, squashing in that change doesn't obviously break anything
>> for me, and is probably a step in the right direction.
>>
>> I'll see what Travis thinks.
>>
>
> We will probably receive build breaks...

Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489

Just tried again on one of those failures (x600) with the the default
removed and just set the on board that uses CONFIG_EXT, but that then
fails at config time.

TBH I'd actually like to take it out of Kconfig (which I realise is
the wrong direction) as it just feels fundamentally wrong the way it
is. I don't know what the U-Boot approach configuration like this
is... struggling to find prior art.
Lukasz Majewski Feb. 12, 2018, 8:48 a.m. | #5
Hi Alex,

> On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
> wrote:
> > On Sun, 11 Feb 2018 21:04:46 +0000
> > Alex Kiernan <alex.kiernan@gmail.com> wrote:
> >  
> >>
> >> That said, squashing in that change doesn't obviously break
> >> anything for me, and is probably a step in the right direction.
> >>
> >> I'll see what Travis thinks.
> >>  
> >
> > We will probably receive build breaks...  
> 
> Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
> 
> Just tried again on one of those failures (x600) with the the default
> removed and just set the on board that uses CONFIG_EXT, but that then
> fails at config time.

Ok. I see

> 
> TBH I'd actually like to take it out of Kconfig (which I realise is
> the wrong direction) as it just feels fundamentally wrong the way it
> is. 

To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to add its
definition to each eligible ./configs/<board>_defconfig file.

Then we would have the previous behaviour preserved. 

> I don't know what the U-Boot approach configuration like this
> is... struggling to find prior art.

Let's ask Tom for his opinion as he did much such work before.

> 


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
Lukasz Majewski Feb. 12, 2018, 10:27 a.m. | #6
On Mon, 12 Feb 2018 09:48:13 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Alex,
> 
> > On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > > On Sun, 11 Feb 2018 21:04:46 +0000
> > > Alex Kiernan <alex.kiernan@gmail.com> wrote:
> > >    
> > >>
> > >> That said, squashing in that change doesn't obviously break
> > >> anything for me, and is probably a step in the right direction.
> > >>
> > >> I'll see what Travis thinks.
> > >>    
> > >
> > > We will probably receive build breaks...    
> > 
> > Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
> > 
> > Just tried again on one of those failures (x600) with the the
> > default removed and just set the on board that uses CONFIG_EXT, but
> > that then fails at config time.  
> 
> Ok. I see
> 
> > 
> > TBH I'd actually like to take it out of Kconfig (which I realise is
> > the wrong direction) as it just feels fundamentally wrong the way it
> > is.   
> 
> To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to add
> its definition to each eligible ./configs/<board>_defconfig file.
> 
> Then we would have the previous behaviour preserved. 
> 
> > I don't know what the U-Boot approach configuration like this
> > is... struggling to find prior art.  

I had similar issue with HW_WATCHDOG conversion:

http://patchwork.ozlabs.org/cover/871576/
especially:

http://patchwork.ozlabs.org/patch/871579/

> 
> Let's ask Tom for his opinion as he did much such work before.
> 
> >   
> 
> 
> 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
Alex Kiernan Feb. 13, 2018, 9:51 a.m. | #7
On Mon, Feb 12, 2018 at 8:48 AM, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Alex,
>
>> On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
>> wrote:
>> > On Sun, 11 Feb 2018 21:04:46 +0000
>> > Alex Kiernan <alex.kiernan@gmail.com> wrote:
>> >
>> >>
>> >> That said, squashing in that change doesn't obviously break
>> >> anything for me, and is probably a step in the right direction.
>> >>
>> >> I'll see what Travis thinks.
>> >>
>> >
>> > We will probably receive build breaks...
>>
>> Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
>>
>> Just tried again on one of those failures (x600) with the the default
>> removed and just set the on board that uses CONFIG_EXT, but that then
>> fails at config time.
>
> Ok. I see
>
>>
>> TBH I'd actually like to take it out of Kconfig (which I realise is
>> the wrong direction) as it just feels fundamentally wrong the way it
>> is.
>
> To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to add its
> definition to each eligible ./configs/<board>_defconfig file.
>
> Then we would have the previous behaviour preserved.
>
>> I don't know what the U-Boot approach configuration like this
>> is... struggling to find prior art.
>
> Let's ask Tom for his opinion as he did much such work before.
>

Thoughts:

- Do nothing, leave CONFIG_SYS_BOOTCOUNT_ADDR as a purely CONFIG_EXT
piece of Kconfig
- Rename CONFIG_SYS_BOOTCOUNT_ADDR to something like
CONFIG_SYS_BOOTCOUNT_ADDR_EXT for just CONFIG_EXT
- Remove CONFIG_SYS_BOOTCOUNT_ADDR from Kconfig altogether (and rename
to say SYS_BOOTCOUNT_ADDR)
- Pick through every config building defaults - okay for some boards,
but lots have it defined based on other memory offsets

I think the only real options are the last two.

Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants splitting into
at least two:

- I2C - an offset from an I2C base for the bootcounter
- Others - an actual address used for storing the bootcounter

I'm struggling to see why EXT is the way it is - AFAICS the location
it uses to access/return the bootcounter is basically local to the two
functions in bootcount_ext and could just be a local variable.
Lukasz Majewski Feb. 13, 2018, 2:41 p.m. | #8
Hi Alex,

> On Mon, Feb 12, 2018 at 8:48 AM, Lukasz Majewski <lukma@denx.de>
> wrote:
> > Hi Alex,
> >  
> >> On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
> >> wrote:  
> >> > On Sun, 11 Feb 2018 21:04:46 +0000
> >> > Alex Kiernan <alex.kiernan@gmail.com> wrote:
> >> >  
> >> >>
> >> >> That said, squashing in that change doesn't obviously break
> >> >> anything for me, and is probably a step in the right direction.
> >> >>
> >> >> I'll see what Travis thinks.
> >> >>  
> >> >
> >> > We will probably receive build breaks...  
> >>
> >> Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
> >>
> >> Just tried again on one of those failures (x600) with the the
> >> default removed and just set the on board that uses CONFIG_EXT,
> >> but that then fails at config time.  
> >
> > Ok. I see
> >  
> >>
> >> TBH I'd actually like to take it out of Kconfig (which I realise is
> >> the wrong direction) as it just feels fundamentally wrong the way
> >> it is.  
> >
> > To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to add
> > its definition to each eligible ./configs/<board>_defconfig file.
> >
> > Then we would have the previous behaviour preserved.
> >  
> >> I don't know what the U-Boot approach configuration like this
> >> is... struggling to find prior art.  
> >
> > Let's ask Tom for his opinion as he did much such work before.
> >  
> 
> Thoughts:
> 
> - Do nothing, leave CONFIG_SYS_BOOTCOUNT_ADDR as a purely CONFIG_EXT
> piece of Kconfig
> - Rename CONFIG_SYS_BOOTCOUNT_ADDR to something like
> CONFIG_SYS_BOOTCOUNT_ADDR_EXT for just CONFIG_EXT
> - Remove CONFIG_SYS_BOOTCOUNT_ADDR from Kconfig altogether (and rename
> to say SYS_BOOTCOUNT_ADDR)
> - Pick through every config building defaults - okay for some boards,
> but lots have it defined based on other memory offsets
> 
> I think the only real options are the last two.

I would go for third option - Remove the SYS_BOOTCOUNT_ADDR from
Kconfig (also for EXT).

> 
> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants splitting into
> at least two:
> 
> - I2C - an offset from an I2C base for the bootcounter
  - RAM/SoC memory - location of special register to store bootcounter
> - Others - an actual address used for storing the bootcounter



> 
> I'm struggling to see why EXT is the way it is - AFAICS the location
> it uses to access/return the bootcounter is basically local to the two
> functions in bootcount_ext and could just be a local variable.
> 

Maybe we can replace it with local, static variable then?


As said above - I would remove the (wrongly?) used BOOTCOUNT_ADDR in
Kconfig. 

Then, for next merge window we can play around with moving
CONFIG_SYS_BOOTCOUNT_ADDR per boards if needed. 

I just do have a feeling that -rc2 is too late for it.... 

Also, wrong SYS_BOOTCOUNT_ADDR will not be caught in build testing and
may cause boards to silency break.


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
Alex Kiernan Feb. 14, 2018, 6:23 a.m. | #9
On Tue, Feb 13, 2018 at 2:41 PM, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Alex,
>
>> On Mon, Feb 12, 2018 at 8:48 AM, Lukasz Majewski <lukma@denx.de>
>> wrote:
>> > Hi Alex,
>> >
>> >> On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
>> >> wrote:
>> >> > On Sun, 11 Feb 2018 21:04:46 +0000
>> >> > Alex Kiernan <alex.kiernan@gmail.com> wrote:
>> >> >
>> >> >>
>> >> >> That said, squashing in that change doesn't obviously break
>> >> >> anything for me, and is probably a step in the right direction.
>> >> >>
>> >> >> I'll see what Travis thinks.
>> >> >>
>> >> >
>> >> > We will probably receive build breaks...
>> >>
>> >> Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
>> >>
>> >> Just tried again on one of those failures (x600) with the the
>> >> default removed and just set the on board that uses CONFIG_EXT,
>> >> but that then fails at config time.
>> >
>> > Ok. I see
>> >
>> >>
>> >> TBH I'd actually like to take it out of Kconfig (which I realise is
>> >> the wrong direction) as it just feels fundamentally wrong the way
>> >> it is.
>> >
>> > To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to add
>> > its definition to each eligible ./configs/<board>_defconfig file.
>> >
>> > Then we would have the previous behaviour preserved.
>> >
>> >> I don't know what the U-Boot approach configuration like this
>> >> is... struggling to find prior art.
>> >
>> > Let's ask Tom for his opinion as he did much such work before.
>> >
>>
>> Thoughts:
>>
>> - Do nothing, leave CONFIG_SYS_BOOTCOUNT_ADDR as a purely CONFIG_EXT
>> piece of Kconfig
>> - Rename CONFIG_SYS_BOOTCOUNT_ADDR to something like
>> CONFIG_SYS_BOOTCOUNT_ADDR_EXT for just CONFIG_EXT
>> - Remove CONFIG_SYS_BOOTCOUNT_ADDR from Kconfig altogether (and rename
>> to say SYS_BOOTCOUNT_ADDR)
>> - Pick through every config building defaults - okay for some boards,
>> but lots have it defined based on other memory offsets
>>
>> I think the only real options are the last two.
>
> I would go for third option - Remove the SYS_BOOTCOUNT_ADDR from
> Kconfig (also for EXT).
>

I agree.

>>
>> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants splitting into
>> at least two:
>>
>> - I2C - an offset from an I2C base for the bootcounter
>   - RAM/SoC memory - location of special register to store bootcounter
>> - Others - an actual address used for storing the bootcounter
>
>
>
>>
>> I'm struggling to see why EXT is the way it is - AFAICS the location
>> it uses to access/return the bootcounter is basically local to the two
>> functions in bootcount_ext and could just be a local variable.
>>
>
> Maybe we can replace it with local, static variable then?
>
>
> As said above - I would remove the (wrongly?) used BOOTCOUNT_ADDR in
> Kconfig.
>

Just removing SYS_BOOTCOUNT_ADDR from Kconfig and putting the default
back into bootcount_ext seems like the simplest correct change. I'll
add that onto the series and throw it at Travis.

> Then, for next merge window we can play around with moving
> CONFIG_SYS_BOOTCOUNT_ADDR per boards if needed.
>
> I just do have a feeling that -rc2 is too late for it....
>
> Also, wrong SYS_BOOTCOUNT_ADDR will not be caught in build testing and
> may cause boards to silency break.
>

Yeah...
Lukasz Majewski Feb. 14, 2018, 8:53 a.m. | #10
Hi Alex,

> On Tue, Feb 13, 2018 at 2:41 PM, Lukasz Majewski <lukma@denx.de>
> wrote:
> > Hi Alex,
> >  
> >> On Mon, Feb 12, 2018 at 8:48 AM, Lukasz Majewski <lukma@denx.de>
> >> wrote:  
> >> > Hi Alex,
> >> >  
> >> >> On Sun, Feb 11, 2018 at 9:44 PM, Lukasz Majewski <lukma@denx.de>
> >> >> wrote:  
> >> >> > On Sun, 11 Feb 2018 21:04:46 +0000
> >> >> > Alex Kiernan <alex.kiernan@gmail.com> wrote:
> >> >> >  
> >> >> >>
> >> >> >> That said, squashing in that change doesn't obviously break
> >> >> >> anything for me, and is probably a step in the right
> >> >> >> direction.
> >> >> >>
> >> >> >> I'll see what Travis thinks.
> >> >> >>  
> >> >> >
> >> >> > We will probably receive build breaks...  
> >> >>
> >> >> Yup... https://travis-ci.org/akiernan/u-boot/jobs/340344489
> >> >>
> >> >> Just tried again on one of those failures (x600) with the the
> >> >> default removed and just set the on board that uses CONFIG_EXT,
> >> >> but that then fails at config time.  
> >> >
> >> > Ok. I see
> >> >  
> >> >>
> >> >> TBH I'd actually like to take it out of Kconfig (which I
> >> >> realise is the wrong direction) as it just feels fundamentally
> >> >> wrong the way it is.  
> >> >
> >> > To finish moving SYS_BOOTCOUNT_ADDR to Kconfig we would need to
> >> > add its definition to each eligible ./configs/<board>_defconfig
> >> > file.
> >> >
> >> > Then we would have the previous behaviour preserved.
> >> >  
> >> >> I don't know what the U-Boot approach configuration like this
> >> >> is... struggling to find prior art.  
> >> >
> >> > Let's ask Tom for his opinion as he did much such work before.
> >> >  
> >>
> >> Thoughts:
> >>
> >> - Do nothing, leave CONFIG_SYS_BOOTCOUNT_ADDR as a purely
> >> CONFIG_EXT piece of Kconfig
> >> - Rename CONFIG_SYS_BOOTCOUNT_ADDR to something like
> >> CONFIG_SYS_BOOTCOUNT_ADDR_EXT for just CONFIG_EXT
> >> - Remove CONFIG_SYS_BOOTCOUNT_ADDR from Kconfig altogether (and
> >> rename to say SYS_BOOTCOUNT_ADDR)
> >> - Pick through every config building defaults - okay for some
> >> boards, but lots have it defined based on other memory offsets
> >>
> >> I think the only real options are the last two.  
> >
> > I would go for third option - Remove the SYS_BOOTCOUNT_ADDR from
> > Kconfig (also for EXT).
> >  
> 
> I agree.
> 
> >>
> >> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants splitting
> >> into at least two:
> >>
> >> - I2C - an offset from an I2C base for the bootcounter  
> >   - RAM/SoC memory - location of special register to store
> > bootcounter  
> >> - Others - an actual address used for storing the bootcounter  
> >
> >
> >  
> >>
> >> I'm struggling to see why EXT is the way it is - AFAICS the
> >> location it uses to access/return the bootcounter is basically
> >> local to the two functions in bootcount_ext and could just be a
> >> local variable. 
> >
> > Maybe we can replace it with local, static variable then?
> >
> >
> > As said above - I would remove the (wrongly?) used BOOTCOUNT_ADDR in
> > Kconfig.
> >  
> 
> Just removing SYS_BOOTCOUNT_ADDR from Kconfig and putting the default
> back into bootcount_ext seems like the simplest correct change. I'll
> add that onto the series and throw it at Travis.

Great. I'm looking forward for next verison of the code - as I do have
some code to be put on top of it.

Thanks for help.

> 
> > Then, for next merge window we can play around with moving
> > CONFIG_SYS_BOOTCOUNT_ADDR per boards if needed.
> >
> > I just do have a feeling that -rc2 is too late for it....
> >
> > Also, wrong SYS_BOOTCOUNT_ADDR will not be caught in build testing
> > and may cause boards to silency break.
> >  
> 
> Yeah...
> 



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
Alex Kiernan Feb. 14, 2018, 4:34 p.m. | #11
On Wed, Feb 14, 2018 at 8:53 AM, Lukasz Majewski <lukma@denx.de> wrote:
>> >> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants splitting
>> >> into at least two:
>> >>
>> >> - I2C - an offset from an I2C base for the bootcounter
>> >   - RAM/SoC memory - location of special register to store
>> > bootcounter
>> >> - Others - an actual address used for storing the bootcounter
>> >
>> >
>> >
>> >>
>> >> I'm struggling to see why EXT is the way it is - AFAICS the
>> >> location it uses to access/return the bootcounter is basically
>> >> local to the two functions in bootcount_ext and could just be a
>> >> local variable.
>> >
>> > Maybe we can replace it with local, static variable then?
>> >
>> >
>> > As said above - I would remove the (wrongly?) used BOOTCOUNT_ADDR in
>> > Kconfig.
>> >
>>
>> Just removing SYS_BOOTCOUNT_ADDR from Kconfig and putting the default
>> back into bootcount_ext seems like the simplest correct change. I'll
>> add that onto the series and throw it at Travis.
>
> Great. I'm looking forward for next verison of the code - as I do have
> some code to be put on top of it.
>

So the super-trivial approach doesn't work, because
CONFIG_SYS_BOOTCOUNT_ADDR isn't in the whitelist anymore
(7af2e3648f3f6d726f6476745c2eec5de709a702) and I think the only reason
it was getting through before is because scripts/check-config.sh
doesn't parse conditionals in Kconfig so thought it was allowed :(

I expect reintroducing CONFIG_SYS_BOOTCOUNT_ADDR to
config_whitelist.txt would "fix" the problem, but that's not going to
make it in.

Which I think means I have to do the work to migrate it out of CONFIG_
land properly.
Lukasz Majewski Feb. 14, 2018, 7:13 p.m. | #12
Hi Alex,

> On Wed, Feb 14, 2018 at 8:53 AM, Lukasz Majewski <lukma@denx.de>
> wrote:
> >> >> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants
> >> >> splitting into at least two:
> >> >>
> >> >> - I2C - an offset from an I2C base for the bootcounter  
> >> >   - RAM/SoC memory - location of special register to store
> >> > bootcounter  
> >> >> - Others - an actual address used for storing the bootcounter  
> >> >
> >> >
> >> >  
> >> >>
> >> >> I'm struggling to see why EXT is the way it is - AFAICS the
> >> >> location it uses to access/return the bootcounter is basically
> >> >> local to the two functions in bootcount_ext and could just be a
> >> >> local variable.  
> >> >
> >> > Maybe we can replace it with local, static variable then?
> >> >
> >> >
> >> > As said above - I would remove the (wrongly?) used
> >> > BOOTCOUNT_ADDR in Kconfig.
> >> >  
> >>
> >> Just removing SYS_BOOTCOUNT_ADDR from Kconfig and putting the
> >> default back into bootcount_ext seems like the simplest correct
> >> change. I'll add that onto the series and throw it at Travis.  
> >
> > Great. I'm looking forward for next verison of the code - as I do
> > have some code to be put on top of it.
> >  
> 
> So the super-trivial approach doesn't work, because
> CONFIG_SYS_BOOTCOUNT_ADDR isn't in the whitelist anymore
> (7af2e3648f3f6d726f6476745c2eec5de709a702) and I think the only reason
> it was getting through before is because scripts/check-config.sh
> doesn't parse conditionals in Kconfig so thought it was allowed :(
> 
> I expect reintroducing CONFIG_SYS_BOOTCOUNT_ADDR to
> config_whitelist.txt would "fix" the problem, but that's not going to
> make it in.
> 
> Which I think means I have to do the work to migrate it out of CONFIG_
> land properly.
> 

Ok. I see.


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
Alex Kiernan Feb. 15, 2018, 2:04 p.m. | #13
On Wed, Feb 14, 2018 at 7:13 PM, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Alex,
>
>> On Wed, Feb 14, 2018 at 8:53 AM, Lukasz Majewski <lukma@denx.de>
>> wrote:
>> >> >> Whatever we do, I think CONFIG_SYS_BOOTCOUNT_ADDR wants
>> >> >> splitting into at least two:
>> >> >>
>> >> >> - I2C - an offset from an I2C base for the bootcounter
>> >> >   - RAM/SoC memory - location of special register to store
>> >> > bootcounter
>> >> >> - Others - an actual address used for storing the bootcounter
>> >> >
>> >> >
>> >> >
>> >> >>
>> >> >> I'm struggling to see why EXT is the way it is - AFAICS the
>> >> >> location it uses to access/return the bootcounter is basically
>> >> >> local to the two functions in bootcount_ext and could just be a
>> >> >> local variable.
>> >> >
>> >> > Maybe we can replace it with local, static variable then?
>> >> >
>> >> >
>> >> > As said above - I would remove the (wrongly?) used
>> >> > BOOTCOUNT_ADDR in Kconfig.
>> >> >
>> >>
>> >> Just removing SYS_BOOTCOUNT_ADDR from Kconfig and putting the
>> >> default back into bootcount_ext seems like the simplest correct
>> >> change. I'll add that onto the series and throw it at Travis.
>> >
>> > Great. I'm looking forward for next verison of the code - as I do
>> > have some code to be put on top of it.
>> >
>>
>> So the super-trivial approach doesn't work, because
>> CONFIG_SYS_BOOTCOUNT_ADDR isn't in the whitelist anymore
>> (7af2e3648f3f6d726f6476745c2eec5de709a702) and I think the only reason
>> it was getting through before is because scripts/check-config.sh
>> doesn't parse conditionals in Kconfig so thought it was allowed :(
>>
>> I expect reintroducing CONFIG_SYS_BOOTCOUNT_ADDR to
>> config_whitelist.txt would "fix" the problem, but that's not going to
>> make it in.
>>
>> Which I think means I have to do the work to migrate it out of CONFIG_
>> land properly.
>>
>
> Ok. I see.
>

'tis done. Only I've fallen foul of the recipient limit on the mailing
list :( Will see if the two pieces get pushed out, if not I'll go and
trim the list and try again.

As you say, this isn't a patch for -rc2

Patch

diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index e0d1fc2..9fde2f2 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -14,6 +14,16 @@  choice
 	prompt "Boot count device"
 	default BOOTCOUNT_AM33XX if AM33XX || SOC_DA8XX
 	default BOOTCOUNT_AT91 if AT91SAM9XE
+	default BOOTCOUNT_GENERIC
+
+config BOOTCOUNT_GENERIC
+	bool "Generic default boot counter"
+	help
+	  Generic bootcount stored at SYS_BOOTCOUNT_ADDR.
+
+	  SYS_BOOTCOUNT_ADDR:
+	    Set to the address where the bootcount and bootcount magic
+	    will be stored.
 
 config BOOTCOUNT_EXT
 	bool "Boot counter on EXT filesystem"
@@ -64,6 +74,7 @@  endchoice
 
 config SYS_BOOTCOUNT_SINGLEWORD
 	bool "Use single word to pack boot count and magic value"
+	depends on BOOTCOUNT_GENERIC
 	help
 	  This option enables packing boot count magic value and boot count
 	  into single word (32 bits).
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index a3658c1..3e1ae8c 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -2,7 +2,7 @@ 
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
-obj-y				+= bootcount.o
+obj-$(CONFIG_BOOTCOUNT_GENERIC)	+= bootcount.o
 obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
 obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o