[U-Boot] [PATCH 8/8] Migrate generic bootcount to Kconfig
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
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 bootcountmagic
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
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"helpGeneric bootcount stored at SYS_BOOTCOUNT_ADDR.SYS_BOOTCOUNT_ADDR:Set to the address where the bootcount and bootcountmagic
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 andboot 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"helpGeneric bootcount stored at SYS_BOOTCOUNT_ADDR.SYS_BOOTCOUNT_ADDR:Set to the address where the bootcount and bootcountmagic
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 andboot 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
participants (2)
-
Alex Kiernan -
Lukasz Majewski