| 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 | expand |
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.
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.
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
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.
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
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
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.
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
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...
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
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.
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
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
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
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(-)