diff mbox series

bootcounter: add DM support for memory based bootcounter

Message ID 20200220092830.1345351-1-hs@denx.de
State Superseded
Headers show
Series bootcounter: add DM support for memory based bootcounter | expand

Commit Message

Heiko Schocher Feb. 20, 2020, 9:28 a.m. UTC
add DM/DTS support for the memory based bootcounter
in drivers/bootcount/bootcount.c.

Let the old implementation in, so boards which have
not yet convert to DM/DTS do not break.

Signed-off-by: Heiko Schocher <hs at denx.de>
---
Travis build:

https://travis-ci.org/hsdenx/u-boot-test/builds/652839618

 doc/device-tree-bindings/misc/bootcounter.txt | 21 +++++
 drivers/bootcount/Kconfig                     |  5 ++
 drivers/bootcount/Makefile                    |  1 +
 drivers/bootcount/bootcount.c                 | 86 +++++++++++++++++++
 4 files changed, 113 insertions(+)
 create mode 100644 doc/device-tree-bindings/misc/bootcounter.txt

Comments

Simon Glass Feb. 21, 2020, 12:50 a.m. UTC | #1
Hi Heiko,

On Thu, 20 Feb 2020 at 02:28, Heiko Schocher <hs at denx.de> wrote:
>
> add DM/DTS support for the memory based bootcounter
> in drivers/bootcount/bootcount.c.
>
> Let the old implementation in, so boards which have
> not yet convert to DM/DTS do not break.
>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> ---
> Travis build:
>
> https://travis-ci.org/hsdenx/u-boot-test/builds/652839618
>
>  doc/device-tree-bindings/misc/bootcounter.txt | 21 +++++
>  drivers/bootcount/Kconfig                     |  5 ++
>  drivers/bootcount/Makefile                    |  1 +
>  drivers/bootcount/bootcount.c                 | 86 +++++++++++++++++++
>  4 files changed, 113 insertions(+)
>  create mode 100644 doc/device-tree-bindings/misc/bootcounter.txt
>
> diff --git a/doc/device-tree-bindings/misc/bootcounter.txt b/doc/device-tree-bindings/misc/bootcounter.txt
> new file mode 100644
> index 0000000000..f4a4a731b9
> --- /dev/null
> +++ b/doc/device-tree-bindings/misc/bootcounter.txt
> @@ -0,0 +1,21 @@
> +U-Boot bootcounter Devicetree Binding
> +=====================================
> +
> +The device tree node describes the U-Boot bootcounter
> +memory based device binding.
> +
> +Required properties :
> +
> +- compatible : "uboot,bootcount";

I think we use u-boot in other bindings

> +- singleword : set this, if you have only one word space
> +    for storing the bootcounter.

single-word

I think this is a boolean property, right?

What is a word? Is it 32 bits? Also, what does it actually mean/do?

> +
> +Example
> +-------
> +
> +MPC83xx based board:
> +
> +bootcount at 0x13ff8 {
> +       compatible = "uboot,bootcount";
> +       reg = <0x13ff8 0x08>;
> +};
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> index 0e506c9ea2..88203607a8 100644
> --- a/drivers/bootcount/Kconfig
> +++ b/drivers/bootcount/Kconfig
> @@ -106,6 +106,11 @@ config DM_BOOTCOUNT_I2C_EEPROM
>           pointing to the underlying i2c eeprom device) and an optional 'offset'
>           property are supported.
>
> +config BOOTCOUNT_MEM
> +       bool "memory based bootcounter"
> +       help
> +         Memory based bootcount, compatible = "uboot,bootcount";
> +
>  endmenu
>
>  endif
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index 73ccfb5a08..059d40d16b 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+
>
>  obj-$(CONFIG_BOOTCOUNT_GENERIC)        += bootcount.o
> +obj-$(CONFIG_BOOTCOUNT_MEM)    += bootcount.o
>  obj-$(CONFIG_BOOTCOUNT_AT91)   += bootcount_at91.o
>  obj-$(CONFIG_BOOTCOUNT_AM33XX) += bootcount_davinci.o
>  obj-$(CONFIG_BOOTCOUNT_RAM)    += bootcount_ram.o
> diff --git a/drivers/bootcount/bootcount.c b/drivers/bootcount/bootcount.c
> index 7a6d03dcca..53bd416cf6 100644
> --- a/drivers/bootcount/bootcount.c
> +++ b/drivers/bootcount/bootcount.c
> @@ -8,6 +8,7 @@
>  #include <cpu_func.h>
>  #include <linux/compiler.h>
>
> +#if !defined(CONFIG_DM_BOOTCOUNT)
>  /* Now implement the generic default functions */
>  __weak void bootcount_store(ulong a)
>  {
> @@ -49,3 +50,88 @@ __weak ulong bootcount_load(void)
>                 return raw_bootcount_load(reg);
>  #endif /* defined(CONFIG_SYS_BOOTCOUNT_SINGLEWORD) */
>  }
> +#else
> +#include <dm.h>
> +

Comment for struct

> +struct bootcount_mem_priv {
> +       phys_addr_t base;
> +       u8 singleword;

bool?
> +};
> +
> +static int bootcount_mem_get(struct udevice *dev, u32 *a)
> +{
> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
> +       void *reg = (void *)priv->base;
> +       u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
> +
> +       if (priv->singleword) {
> +               u32 tmp = raw_bootcount_load(reg);
> +
> +               if ((tmp & 0xffff0000) != (magic & 0xffff0000))
> +                       return -ENODEV;
> +
> +               *a = (tmp & 0x0000ffff);
> +       } else {
> +               if (raw_bootcount_load(reg + 4) != magic)
> +                       return -ENODEV;
> +
> +               *a = raw_bootcount_load(reg);
> +       }
> +
> +       return 0;
> +};
> +
> +static int bootcount_mem_set(struct udevice *dev, const u32 a)
> +{
> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
> +       void *reg = (void *)priv->base;
> +       u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
> +       uintptr_t flush_start = rounddown(priv->base,
> +                                         CONFIG_SYS_CACHELINE_SIZE);
> +       uintptr_t flush_end;
> +
> +       if (priv->singleword) {
> +               raw_bootcount_store(reg, (magic & 0xffff0000) | a);
> +               flush_end = roundup(priv->base + 4,
> +                                   CONFIG_SYS_CACHELINE_SIZE);
> +       } else {
> +               raw_bootcount_store(reg, a);
> +               raw_bootcount_store(reg + 4, magic);
> +               flush_end = roundup(priv->base + 8,
> +                                   CONFIG_SYS_CACHELINE_SIZE);
> +       }
> +       flush_dcache_range(flush_start, flush_end);
> +
> +       return 0;
> +};
> +
> +static const struct bootcount_ops bootcount_mem_ops = {
> +       .get = bootcount_mem_get,
> +       .set = bootcount_mem_set,
> +};
> +
> +static int bootcount_mem_probe(struct udevice *dev)
> +{
> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
> +
> +       priv->base = (phys_addr_t)devfdt_get_addr(dev);

dev_read_addr()

> +       priv->singleword = dev_read_u32_default(dev, "singleword", 0);

dev_read_bool() ?

> +
> +       return 0;
> +}
> +
> +static const struct udevice_id bootcount_mem_ids[] = {
> +       { .compatible = "uboot,bootcount" },
> +       { .compatible = "u-boot,bootcount" },

Can we just have the second one?

> +       { }
> +};
> +
> +U_BOOT_DRIVER(bootcount_mem) = {
> +       .name   = "bootcount-mem",
> +       .id     = UCLASS_BOOTCOUNT,
> +       .priv_auto_alloc_size = sizeof(struct bootcount_mem_priv),
> +       .probe  = bootcount_mem_probe,
> +       .of_match = bootcount_mem_ids,
> +       .ops    = &bootcount_mem_ops,
> +};
> +#endif
> --
> 2.24.1
>

Regards,
Simon
Heiko Schocher Feb. 22, 2020, 12:17 p.m. UTC | #2
Hello Simon,

Am 21.02.2020 um 01:50 schrieb Simon Glass:
> Hi Heiko,
> 
> On Thu, 20 Feb 2020 at 02:28, Heiko Schocher <hs at denx.de> wrote:
>>
>> add DM/DTS support for the memory based bootcounter
>> in drivers/bootcount/bootcount.c.
>>
>> Let the old implementation in, so boards which have
>> not yet convert to DM/DTS do not break.
>>
>> Signed-off-by: Heiko Schocher <hs at denx.de>
>> ---
>> Travis build:
>>
>> https://travis-ci.org/hsdenx/u-boot-test/builds/652839618
>>
>>   doc/device-tree-bindings/misc/bootcounter.txt | 21 +++++
>>   drivers/bootcount/Kconfig                     |  5 ++
>>   drivers/bootcount/Makefile                    |  1 +
>>   drivers/bootcount/bootcount.c                 | 86 +++++++++++++++++++
>>   4 files changed, 113 insertions(+)
>>   create mode 100644 doc/device-tree-bindings/misc/bootcounter.txt
>>
>> diff --git a/doc/device-tree-bindings/misc/bootcounter.txt b/doc/device-tree-bindings/misc/bootcounter.txt
>> new file mode 100644
>> index 0000000000..f4a4a731b9
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/misc/bootcounter.txt
>> @@ -0,0 +1,21 @@
>> +U-Boot bootcounter Devicetree Binding
>> +=====================================
>> +
>> +The device tree node describes the U-Boot bootcounter
>> +memory based device binding.
>> +
>> +Required properties :
>> +
>> +- compatible : "uboot,bootcount";
> 
> I think we use u-boot in other bindings

Yes. I just porting some mpc83xx boards to DM/DTS and they use
"uboot,bootcounter" ... but I think, it should be possible to change
this in the respectiv DTS files.

> 
>> +- singleword : set this, if you have only one word space
>> +    for storing the bootcounter.
> 
> single-word

Yep

> 
> I think this is a boolean property, right?

Yes.

> What is a word? Is it 32 bits? Also, what does it actually mean/do?

If enabled bootcounter driver does use one word (32 bit) only.

See CONFIG_SYS_BOOTCOUNT_SINGLEWORD in current version of mainline
driver.

>> +
>> +Example
>> +-------
>> +
>> +MPC83xx based board:
>> +
>> +bootcount at 0x13ff8 {
>> +       compatible = "uboot,bootcount";
>> +       reg = <0x13ff8 0x08>;
>> +};
>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>> index 0e506c9ea2..88203607a8 100644
>> --- a/drivers/bootcount/Kconfig
>> +++ b/drivers/bootcount/Kconfig
>> @@ -106,6 +106,11 @@ config DM_BOOTCOUNT_I2C_EEPROM
>>            pointing to the underlying i2c eeprom device) and an optional 'offset'
>>            property are supported.
>>
>> +config BOOTCOUNT_MEM
>> +       bool "memory based bootcounter"
>> +       help
>> +         Memory based bootcount, compatible = "uboot,bootcount";
>> +
>>   endmenu
>>
>>   endif
>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>> index 73ccfb5a08..059d40d16b 100644
>> --- a/drivers/bootcount/Makefile
>> +++ b/drivers/bootcount/Makefile
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0+
>>
>>   obj-$(CONFIG_BOOTCOUNT_GENERIC)        += bootcount.o
>> +obj-$(CONFIG_BOOTCOUNT_MEM)    += bootcount.o
>>   obj-$(CONFIG_BOOTCOUNT_AT91)   += bootcount_at91.o
>>   obj-$(CONFIG_BOOTCOUNT_AM33XX) += bootcount_davinci.o
>>   obj-$(CONFIG_BOOTCOUNT_RAM)    += bootcount_ram.o
>> diff --git a/drivers/bootcount/bootcount.c b/drivers/bootcount/bootcount.c
>> index 7a6d03dcca..53bd416cf6 100644
>> --- a/drivers/bootcount/bootcount.c
>> +++ b/drivers/bootcount/bootcount.c
>> @@ -8,6 +8,7 @@
>>   #include <cpu_func.h>
>>   #include <linux/compiler.h>
>>
>> +#if !defined(CONFIG_DM_BOOTCOUNT)
>>   /* Now implement the generic default functions */
>>   __weak void bootcount_store(ulong a)
>>   {
>> @@ -49,3 +50,88 @@ __weak ulong bootcount_load(void)
>>                  return raw_bootcount_load(reg);
>>   #endif /* defined(CONFIG_SYS_BOOTCOUNT_SINGLEWORD) */
>>   }
>> +#else
>> +#include <dm.h>
>> +
> 
> Comment for struct

Of course, I add one, sorry for this.

>> +struct bootcount_mem_priv {
>> +       phys_addr_t base;
>> +       u8 singleword;
> 
> bool?

Yes, I change this to bool.

>> +};
>> +
>> +static int bootcount_mem_get(struct udevice *dev, u32 *a)
>> +{
>> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
>> +       void *reg = (void *)priv->base;
>> +       u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
>> +
>> +       if (priv->singleword) {
>> +               u32 tmp = raw_bootcount_load(reg);
>> +
>> +               if ((tmp & 0xffff0000) != (magic & 0xffff0000))
>> +                       return -ENODEV;
>> +
>> +               *a = (tmp & 0x0000ffff);
>> +       } else {
>> +               if (raw_bootcount_load(reg + 4) != magic)
>> +                       return -ENODEV;
>> +
>> +               *a = raw_bootcount_load(reg);
>> +       }
>> +
>> +       return 0;
>> +};
>> +
>> +static int bootcount_mem_set(struct udevice *dev, const u32 a)
>> +{
>> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
>> +       void *reg = (void *)priv->base;
>> +       u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
>> +       uintptr_t flush_start = rounddown(priv->base,
>> +                                         CONFIG_SYS_CACHELINE_SIZE);
>> +       uintptr_t flush_end;
>> +
>> +       if (priv->singleword) {
>> +               raw_bootcount_store(reg, (magic & 0xffff0000) | a);
>> +               flush_end = roundup(priv->base + 4,
>> +                                   CONFIG_SYS_CACHELINE_SIZE);
>> +       } else {
>> +               raw_bootcount_store(reg, a);
>> +               raw_bootcount_store(reg + 4, magic);
>> +               flush_end = roundup(priv->base + 8,
>> +                                   CONFIG_SYS_CACHELINE_SIZE);
>> +       }
>> +       flush_dcache_range(flush_start, flush_end);
>> +
>> +       return 0;
>> +};
>> +
>> +static const struct bootcount_ops bootcount_mem_ops = {
>> +       .get = bootcount_mem_get,
>> +       .set = bootcount_mem_set,
>> +};
>> +
>> +static int bootcount_mem_probe(struct udevice *dev)
>> +{
>> +       struct bootcount_mem_priv *priv = dev_get_priv(dev);
>> +
>> +       priv->base = (phys_addr_t)devfdt_get_addr(dev);
> 
> dev_read_addr()

Ok, I check it with dev_read_addr()

> 
>> +       priv->singleword = dev_read_u32_default(dev, "singleword", 0);
> 
> dev_read_bool() ?

Yep.

> 
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct udevice_id bootcount_mem_ids[] = {
>> +       { .compatible = "uboot,bootcount" },
>> +       { .compatible = "u-boot,bootcount" },
> 
> Can we just have the second one?

I ask, if we can change the DTS files they use.

> 
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(bootcount_mem) = {
>> +       .name   = "bootcount-mem",
>> +       .id     = UCLASS_BOOTCOUNT,
>> +       .priv_auto_alloc_size = sizeof(struct bootcount_mem_priv),
>> +       .probe  = bootcount_mem_probe,
>> +       .of_match = bootcount_mem_ids,
>> +       .ops    = &bootcount_mem_ops,
>> +};
>> +#endif
>> --
>> 2.24.1
>>
> 
> Regards,
> Simon

Thanks for the review.

bye,
Heiko
>
diff mbox series

Patch

diff --git a/doc/device-tree-bindings/misc/bootcounter.txt b/doc/device-tree-bindings/misc/bootcounter.txt
new file mode 100644
index 0000000000..f4a4a731b9
--- /dev/null
+++ b/doc/device-tree-bindings/misc/bootcounter.txt
@@ -0,0 +1,21 @@ 
+U-Boot bootcounter Devicetree Binding
+=====================================
+
+The device tree node describes the U-Boot bootcounter
+memory based device binding.
+
+Required properties :
+
+- compatible : "uboot,bootcount";
+- singleword : set this, if you have only one word space
+    for storing the bootcounter.
+
+Example
+-------
+
+MPC83xx based board:
+
+bootcount at 0x13ff8 {
+	compatible = "uboot,bootcount";
+	reg = <0x13ff8 0x08>;
+};
diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index 0e506c9ea2..88203607a8 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -106,6 +106,11 @@  config DM_BOOTCOUNT_I2C_EEPROM
 	  pointing to the underlying i2c eeprom device) and an optional 'offset'
 	  property are supported.
 
+config BOOTCOUNT_MEM
+	bool "memory based bootcounter"
+	help
+	  Memory based bootcount, compatible = "uboot,bootcount";
+
 endmenu
 
 endif
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index 73ccfb5a08..059d40d16b 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -1,6 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0+
 
 obj-$(CONFIG_BOOTCOUNT_GENERIC)	+= bootcount.o
+obj-$(CONFIG_BOOTCOUNT_MEM)	+= bootcount.o
 obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
 obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o
diff --git a/drivers/bootcount/bootcount.c b/drivers/bootcount/bootcount.c
index 7a6d03dcca..53bd416cf6 100644
--- a/drivers/bootcount/bootcount.c
+++ b/drivers/bootcount/bootcount.c
@@ -8,6 +8,7 @@ 
 #include <cpu_func.h>
 #include <linux/compiler.h>
 
+#if !defined(CONFIG_DM_BOOTCOUNT)
 /* Now implement the generic default functions */
 __weak void bootcount_store(ulong a)
 {
@@ -49,3 +50,88 @@  __weak ulong bootcount_load(void)
 		return raw_bootcount_load(reg);
 #endif /* defined(CONFIG_SYS_BOOTCOUNT_SINGLEWORD) */
 }
+#else
+#include <dm.h>
+
+struct bootcount_mem_priv {
+	phys_addr_t base;
+	u8 singleword;
+};
+
+static int bootcount_mem_get(struct udevice *dev, u32 *a)
+{
+	struct bootcount_mem_priv *priv = dev_get_priv(dev);
+	void *reg = (void *)priv->base;
+	u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
+
+	if (priv->singleword) {
+		u32 tmp = raw_bootcount_load(reg);
+
+		if ((tmp & 0xffff0000) != (magic & 0xffff0000))
+			return -ENODEV;
+
+		*a = (tmp & 0x0000ffff);
+	} else {
+		if (raw_bootcount_load(reg + 4) != magic)
+			return -ENODEV;
+
+		*a = raw_bootcount_load(reg);
+	}
+
+	return 0;
+};
+
+static int bootcount_mem_set(struct udevice *dev, const u32 a)
+{
+	struct bootcount_mem_priv *priv = dev_get_priv(dev);
+	void *reg = (void *)priv->base;
+	u32 magic = CONFIG_SYS_BOOTCOUNT_MAGIC;
+	uintptr_t flush_start = rounddown(priv->base,
+					  CONFIG_SYS_CACHELINE_SIZE);
+	uintptr_t flush_end;
+
+	if (priv->singleword) {
+		raw_bootcount_store(reg, (magic & 0xffff0000) | a);
+		flush_end = roundup(priv->base + 4,
+				    CONFIG_SYS_CACHELINE_SIZE);
+	} else {
+		raw_bootcount_store(reg, a);
+		raw_bootcount_store(reg + 4, magic);
+		flush_end = roundup(priv->base + 8,
+				    CONFIG_SYS_CACHELINE_SIZE);
+	}
+	flush_dcache_range(flush_start, flush_end);
+
+	return 0;
+};
+
+static const struct bootcount_ops bootcount_mem_ops = {
+	.get = bootcount_mem_get,
+	.set = bootcount_mem_set,
+};
+
+static int bootcount_mem_probe(struct udevice *dev)
+{
+	struct bootcount_mem_priv *priv = dev_get_priv(dev);
+
+	priv->base = (phys_addr_t)devfdt_get_addr(dev);
+	priv->singleword = dev_read_u32_default(dev, "singleword", 0);
+
+	return 0;
+}
+
+static const struct udevice_id bootcount_mem_ids[] = {
+	{ .compatible = "uboot,bootcount" },
+	{ .compatible = "u-boot,bootcount" },
+	{ }
+};
+
+U_BOOT_DRIVER(bootcount_mem) = {
+	.name	= "bootcount-mem",
+	.id	= UCLASS_BOOTCOUNT,
+	.priv_auto_alloc_size = sizeof(struct bootcount_mem_priv),
+	.probe	= bootcount_mem_probe,
+	.of_match = bootcount_mem_ids,
+	.ops	= &bootcount_mem_ops,
+};
+#endif