[PATCH v4 0/4] Bootcount driver using NVMEM cell as backend.

[PATCH v4 0/4] Bootcount driver using NVMEM cell as backend.

[Nandor Han]

Description
-----------
Implement a bootcount (1) related driver that uses
NVMEM as a backend. The patchset will also update the
`snvs_lpgpr` driver to support 2 bytes NVMEM cells.

1. https://www.denx.de/wiki/view/DULG/UBootBootCountLimit

Testing
-------
Hardware: i.MX6sx evaluation board
Kernel: linux-imx-5.4.24

1. Configure the bootcount hardware in DT to use a NVMEM cell provided
by `snvs_lpgpr` driver as backend.
e.g. This will configure a 4 bytes long NVMEM cell
```
  bootcount_regs: bootcount-snvs-regs@0 {
      reg = <0x0 0x04>;
  };
```

2. Record the current NVMEM cell content:
```
~ # hexdump -C
/sys/devices/soc0/soc/2000000.aips-bus/20cc000.snvs/20cc000.snvs:snvs-lpgpr/20cc000.snvs:snvs-lpgpr0/nvmem
00000000  00 00 01 b0                                       |....|
00000004
```

3. Write the bootcount and check that is successful: PASS
```
~ # echo 1 > /sys/bus/platform/drivers/bootcount-nvmem/bootcount/value
bootcount: Write regval: 0xb0010001
~ # hexdump -C
/sys/devices/soc0/soc/2000000.aips-bus/20cc000.snvs/20cc000.snvs:snvs-lpgpr/20cc000.snvs:snvs-lpgpr0/nvmem
00000000  01 00 01 b0                                       |....|
00000004
``` 

Note: similar tests were done also for 2 bytes NVMEM cell size.

Kernel: linux-next, tag: next-20210322
1. Enable bootcount and snvs_lpgpr drivers
2. Verify that they compile succesfully: PASS
```
kernel-master> make -j2 drivers/nvmem/
  DESCEND  objtool
  CALL    scripts/atomic/check-atomics.sh
  CC      arch/x86/kernel/asm-offsets.s
  CALL    scripts/checksyscalls.sh
  CC      drivers/nvmem/core.o
  CC      drivers/nvmem/bootcount-nvmem.o
  CC      drivers/nvmem/snvs_lpgpr.o
  AR      drivers/nvmem/built-in.
```

Testing the bootcount YAML document:
1. Check the document by running the command:
`make DT_CHECKER_FLAGS=-m dt_binding_check`
2. Verify that is successful: PASS
3. Verify that no warnings are generated by bootcount related yaml file: PASS
```
...
CHECK Documentation/devicetree/bindings/nvmem/brcm,nvram.example.dt.yaml                                                                      DTC Documentation/devicetree/bindings/nvmem/bootcount-nvmem.example.dt.yaml
CHECK Documentation/devicetree/bindings/nvmem/bootcount-nvmem.example.dt.yaml
DTC Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.example.dt.yaml
...
```

Changes since v1
----------------
- Fix the dt-bindings YAML document.

Changes since v2
----------------
- Fix the dt-bindings YAML document warnings.

Changes since v3
----------------
- Remove the dependencies from the bindings documentation
- Fix the `bootcount-magic` description from bindings documentation

Nandor Han (4):
  dt-bindings: nvmem: Add bootcount-nvmem
  nvmem: bootcount: add bootcount driver
  nvmem: snvs_lpgpr: use cell stride for regmap size calculation
  nvmem: snvs_lpgpr: support two bytes NVMEM cell size

 .../bindings/nvmem/bootcount-nvmem.yaml       |  71 +++++++
 drivers/nvmem/Kconfig                         |  10 +
 drivers/nvmem/Makefile                        |   1 +
 drivers/nvmem/bootcount-nvmem.c               | 195 ++++++++++++++++++
 drivers/nvmem/snvs_lpgpr.c                    |  67 +++++-
 5 files changed, 338 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/bootcount-nvmem.yaml
 create mode 100644 drivers/nvmem/bootcount-nvmem.c

-- 
2.26.3

[PATCH v4 1/4] dt-bindings: nvmem: Add bootcount-nvmem

[Nandor Han]

Documents the device tree bindings for `bootcount-nvmem` driver.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 .../bindings/nvmem/bootcount-nvmem.yaml       | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/bootcount-nvmem.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/bootcount-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/bootcount-nvmem.yaml
new file mode 100644
index 000000000000..1200ef906843
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/bootcount-nvmem.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright (c) Vaisala Oyj. All rights reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/bootcount-nvmem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bootcount NVMEM bindings
+
+maintainers:
+  - Nandor Han <nandor.han@vaisala.com>
+
+description: |
+  This binding is intendent to describe the hardware location for
+  storing the bootcount value and magic combo.
+
+  The NVMEM cell size should be 2 or 4 bytes.
+
+allOf:
+  - $ref: "nvmem-consumer.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - linux,bootcount-nvmem
+
+  nvmem-cells:
+    description: Phandle to reboot mode nvmem data cell.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  nvmem-cell-names:
+    description: Name of the NVMEM cell.
+    $ref: /schemas/types.yaml#/definitions/string-array
+    enum:
+      - bootcount-regs
+
+  linux,bootcount-magic:
+    description: Override the default magic value.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - nvmem-cells
+  - nvmem-cell-names
+
+additionalProperties: false
+
+examples:
+  # example with 16 bit nvram cell:
+  - |
+    bootcount {
+        compatible = "linux,bootcount-nvmem";
+        nvmem-cells = <&bootcount_regs>;
+        nvmem-cell-names = "bootcount-regs";
+    };
+
+    rtc: rtc@68 {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        reg = <0x68 0x10>;
+
+        bootcount_regs: bootcount_nvmem_regs@e {
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            reg = <0x0e 0x2>;
+        };
+    };
+
+...
-- 
2.26.3

[PATCH v4 2/4] nvmem: bootcount: add bootcount driver

[Nandor Han]

In order to have a robust system we want to be able to identify and take
actions if a boot loop occurs. This is possible by using the bootcount
feature, which can be used to identify the number of times device has
booted since bootcount was last time reset. Bootcount feature (1)
requires a collaboration between bootloader and user-space, where
the bootloader will increase a counter and user-space reset it.
If the counter is not reset and a pre-established threshold is reached,
bootloader can react and take action.

This is the kernel side implementation, which can be used to
identify the number of times device has booted since bootcount was
last time reset.

The driver supports both 16 and 32 bits NVMEM cell size.

1) https://www.denx.de/wiki/DULG/UBootBootCountLimit

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 drivers/nvmem/Kconfig           |  10 ++
 drivers/nvmem/Makefile          |   1 +
 drivers/nvmem/bootcount-nvmem.c | 195 ++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/nvmem/bootcount-nvmem.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index dd2019006838..d5413c937350 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -288,4 +288,14 @@ config NVMEM_BRCM_NVRAM
 	  This driver provides support for Broadcom's NVRAM that can be accessed
 	  using I/O mapping.
 
+config BOOTCOUNT_NVMEM
+	bool "Bootcount driver using nvmem registers"
+	depends on OF
+	depends on NVMEM
+	help
+	  Driver that implements the bootcount feature support using a
+	  NVMEM cell as a backend. The driver supports 2 and 4 bytes
+	  size cells.
+
+	  Say y here to enable bootcount support.
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index bbea1410240a..4c77679bbf0d 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
 nvmem-rmem-y			:= rmem.o
 obj-$(CONFIG_NVMEM_BRCM_NVRAM)	+= nvmem_brcm_nvram.o
 nvmem_brcm_nvram-y		:= brcm_nvram.o
+obj-$(CONFIG_BOOTCOUNT_NVMEM)	+= bootcount-nvmem.o
diff --git a/drivers/nvmem/bootcount-nvmem.c b/drivers/nvmem/bootcount-nvmem.c
new file mode 100644
index 000000000000..7d9b6caefc2b
--- /dev/null
+++ b/drivers/nvmem/bootcount-nvmem.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) Vaisala Oyj. All rights reserved.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Default magic values from u-boot bootcount drivers */
+#define BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL16 0xBC00
+#define BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL32 0xB001C041
+
+struct bootcount_nvmem {
+	struct nvmem_cell *nvmem;
+	u32 magic;
+	u32 mask;
+	size_t bytes_count;
+};
+
+static ssize_t value_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct bootcount_nvmem *bootcount = dev_get_drvdata(dev);
+	u32 regval;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &regval);
+	if (ret < 0)
+		return ret;
+
+	/* Check if the value fits */
+	if ((regval & ~(bootcount->mask)) != 0)
+		return -EINVAL;
+
+	/*
+	 * In case we use 2 bytes for saving the value we need to take
+	 * in consideration the endianness of the system. Because of this
+	 * we mirror the 2 bytes from one side to another.
+	 * This way, regardless of endianness, the value will be written
+	 * in the correct order.
+	 */
+	if (bootcount->bytes_count == 2) {
+		regval &= 0xffff;
+		regval |= (regval & 0xffff) << 16;
+	}
+
+	regval = (~bootcount->mask & bootcount->magic) |
+		 (regval & bootcount->mask);
+	ret = nvmem_cell_write(bootcount->nvmem, &regval,
+			       bootcount->bytes_count);
+	if (ret < 0)
+		return ret;
+	else if (ret != bootcount->bytes_count)
+		ret = -EIO;
+	else
+		ret = count;
+
+	return ret;
+}
+
+static ssize_t value_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct bootcount_nvmem *bootcount = dev_get_drvdata(dev);
+	u32 regval;
+	void *val;
+	size_t len;
+	int ret;
+
+	val = nvmem_cell_read(bootcount->nvmem, &len);
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+
+	if (len != bootcount->bytes_count) {
+		kfree(val);
+		return -EINVAL;
+	}
+
+	if (bootcount->bytes_count == 2)
+		regval = *(u16 *)val;
+	else
+		regval = *(u32 *)val;
+
+	kfree(val);
+
+	if ((regval & ~bootcount->mask) == bootcount->magic)
+		ret = scnprintf(buf, PAGE_SIZE, "%u\n",
+				(unsigned int)(regval & bootcount->mask));
+	else {
+		dev_warn(dev, "invalid magic value\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static DEVICE_ATTR_RW(value);
+
+static int bootcount_nvmem_probe(struct platform_device *pdev)
+{
+	struct bootcount_nvmem *bootcount;
+	int ret;
+	u32 bits;
+	void *val = NULL;
+	size_t len;
+
+	bootcount = devm_kzalloc(&pdev->dev, sizeof(struct bootcount_nvmem),
+				 GFP_KERNEL);
+	if (!bootcount)
+		return -ENOMEM;
+
+	bootcount->nvmem = devm_nvmem_cell_get(&pdev->dev, "bootcount-regs");
+	if (IS_ERR(bootcount->nvmem)) {
+		if (PTR_ERR(bootcount->nvmem) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "cannot get 'bootcount-regs'\n");
+		return PTR_ERR(bootcount->nvmem);
+	}
+
+	/* detect cell dimensions */
+	val = nvmem_cell_read(bootcount->nvmem, &len);
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+	kfree(val);
+	val = NULL;
+
+	if (len != 2 && len != 4) {
+		dev_err(&pdev->dev, "unsupported register size\n");
+		return -EINVAL;
+	}
+
+	bootcount->bytes_count = len;
+
+	platform_set_drvdata(pdev, bootcount);
+
+	ret = device_create_file(&pdev->dev, &dev_attr_value);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to export bootcount value\n");
+		return ret;
+	}
+
+	bits = bootcount->bytes_count << 3;
+	bootcount->mask = GENMASK((bits >> 1) - 1, 0);
+
+	ret = of_property_read_u32(pdev->dev.of_node, "linux,bootcount-magic",
+				   &bootcount->magic);
+	if (ret == -EINVAL) {
+		if (bootcount->bytes_count == 2)
+			bootcount->magic = BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL16;
+		else
+			bootcount->magic = BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL32;
+		ret = 0;
+	} else if (ret) {
+		dev_err(&pdev->dev,
+			"failed to parse linux,bootcount-magic, error: %d\n",
+			ret);
+		return ret;
+	}
+
+	bootcount->magic &= ~bootcount->mask;
+
+	return ret;
+}
+
+static int bootcount_nvmem_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_value);
+
+	return 0;
+}
+
+static const struct of_device_id bootcount_nvmem_match[] = {
+	{ .compatible = "linux,bootcount-nvmem" },
+	{},
+};
+
+static struct platform_driver bootcount_nvmem_driver = {
+	.driver = {
+		.name = "bootcount-nvmem",
+		.of_match_table = bootcount_nvmem_match,
+	},
+	.probe = bootcount_nvmem_probe,
+	.remove = bootcount_nvmem_remove,
+};
+
+module_platform_driver(bootcount_nvmem_driver);
+
+MODULE_DEVICE_TABLE(of, bootcount_nvmem_match);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vaisala Oyj");
+MODULE_DESCRIPTION("Bootcount driver using nvmem compatible registers");
-- 
2.26.3

[PATCH v4 3/4] nvmem: snvs_lpgpr: use cell stride for regmap size calculation

[Nandor Han]

Using a hard-coded value for calculating the number of registers to read
makes future changes more challenging.

Change the calculation to use the NVMEM cell stride instead of a hard
coded value. This will allow specifying different NVMEM cell sizes.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 drivers/nvmem/snvs_lpgpr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvmem/snvs_lpgpr.c b/drivers/nvmem/snvs_lpgpr.c
index 4692aa985bd6..35457421314a 100644
--- a/drivers/nvmem/snvs_lpgpr.c
+++ b/drivers/nvmem/snvs_lpgpr.c
@@ -72,7 +72,7 @@ static int snvs_lpgpr_write(void *context, unsigned int offset, void *val,
 		return -EPERM;
 
 	return regmap_bulk_write(priv->regmap, dcfg->offset + offset, val,
-				bytes / 4);
+				 bytes / priv->cfg.stride);
 }
 
 static int snvs_lpgpr_read(void *context, unsigned int offset, void *val,
@@ -81,8 +81,8 @@ static int snvs_lpgpr_read(void *context, unsigned int offset, void *val,
 	struct snvs_lpgpr_priv *priv = context;
 	const struct snvs_lpgpr_cfg *dcfg = priv->dcfg;
 
-	return regmap_bulk_read(priv->regmap, dcfg->offset + offset,
-			       val, bytes / 4);
+	return regmap_bulk_read(priv->regmap, dcfg->offset + offset, val,
+				bytes / priv->cfg.stride);
 }
 
 static int snvs_lpgpr_probe(struct platform_device *pdev)
-- 
2.26.3

[PATCH v4 4/4] nvmem: snvs_lpgpr: support two bytes NVMEM cell size

[Nandor Han]

In some situation is desired to use less than 4 bytes for storing data.
This will allow using the same register for multiple purposes.

Add support for allowing 2 bytes granularity for NVMEM cells.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 drivers/nvmem/snvs_lpgpr.c | 67 ++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/nvmem/snvs_lpgpr.c b/drivers/nvmem/snvs_lpgpr.c
index 35457421314a..44614f3d68f0 100644
--- a/drivers/nvmem/snvs_lpgpr.c
+++ b/drivers/nvmem/snvs_lpgpr.c
@@ -21,6 +21,9 @@
 #define IMX_GPR_SL		BIT(5)
 #define IMX_GPR_HL		BIT(5)
 
+#define REGMAP_FIELD_SIZE 16
+#define REGMAP_FIELDS_PER_REG 2
+
 struct snvs_lpgpr_cfg {
 	int offset;
 	int offset_hplr;
@@ -33,6 +36,7 @@ struct snvs_lpgpr_priv {
 	struct regmap			*regmap;
 	struct nvmem_config		cfg;
 	const struct snvs_lpgpr_cfg	*dcfg;
+	struct regmap_field **reg_fields;
 };
 
 static const struct snvs_lpgpr_cfg snvs_lpgpr_cfg_imx6q = {
@@ -56,6 +60,11 @@ static int snvs_lpgpr_write(void *context, unsigned int offset, void *val,
 	const struct snvs_lpgpr_cfg *dcfg = priv->dcfg;
 	unsigned int lock_reg;
 	int ret;
+	u32 regval;
+	unsigned int field_id;
+
+	if (offset + bytes > dcfg->size)
+		return -EINVAL;
 
 	ret = regmap_read(priv->regmap, dcfg->offset_hplr, &lock_reg);
 	if (ret < 0)
@@ -71,8 +80,16 @@ static int snvs_lpgpr_write(void *context, unsigned int offset, void *val,
 	if (lock_reg & IMX_GPR_HL)
 		return -EPERM;
 
-	return regmap_bulk_write(priv->regmap, dcfg->offset + offset, val,
-				 bytes / priv->cfg.stride);
+	if (bytes == (REGMAP_FIELD_SIZE >> 3)) {
+		regval = *(u16 *)(val);
+		field_id = offset / REGMAP_FIELDS_PER_REG;
+		ret = regmap_field_write(priv->reg_fields[field_id], regval);
+	} else {
+		ret = regmap_bulk_write(priv->regmap, dcfg->offset + offset,
+					val, bytes / priv->cfg.stride);
+	}
+
+	return ret;
 }
 
 static int snvs_lpgpr_read(void *context, unsigned int offset, void *val,
@@ -80,9 +97,27 @@ static int snvs_lpgpr_read(void *context, unsigned int offset, void *val,
 {
 	struct snvs_lpgpr_priv *priv = context;
 	const struct snvs_lpgpr_cfg *dcfg = priv->dcfg;
+	int ret;
+	u32 regval;
+	unsigned int field_id;
 
-	return regmap_bulk_read(priv->regmap, dcfg->offset + offset, val,
-				bytes / priv->cfg.stride);
+	if (offset + bytes > dcfg->size)
+		return -EINVAL;
+
+	if (bytes == (REGMAP_FIELD_SIZE >> 3)) {
+		field_id = offset / REGMAP_FIELDS_PER_REG;
+		ret = regmap_field_read(priv->reg_fields[field_id], &regval);
+		if (ret)
+			return ret;
+
+		*(u16 *)(val) = regval;
+	} else {
+		ret = regmap_bulk_read(priv->regmap, dcfg->offset + offset, val,
+				       bytes / priv->cfg.stride);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 static int snvs_lpgpr_probe(struct platform_device *pdev)
@@ -94,6 +129,8 @@ static int snvs_lpgpr_probe(struct platform_device *pdev)
 	struct nvmem_config *cfg;
 	struct nvmem_device *nvmem;
 	const struct snvs_lpgpr_cfg *dcfg;
+	int i;
+	int fields_count;
 
 	if (!node)
 		return -ENOENT;
@@ -121,13 +158,31 @@ static int snvs_lpgpr_probe(struct platform_device *pdev)
 	cfg->priv = priv;
 	cfg->name = dev_name(dev);
 	cfg->dev = dev;
-	cfg->stride = 4;
-	cfg->word_size = 4;
+	cfg->stride = 2;
+	cfg->word_size = 2;
 	cfg->size = dcfg->size;
 	cfg->owner = THIS_MODULE;
 	cfg->reg_read  = snvs_lpgpr_read;
 	cfg->reg_write = snvs_lpgpr_write;
 
+	fields_count = priv->dcfg->size / priv->cfg.stride;
+	priv->reg_fields = devm_kzalloc(
+		dev, sizeof(struct regmap_field *) * fields_count, GFP_KERNEL);
+	if (!priv->reg_fields)
+		return -ENOMEM;
+
+	for (i = 0; i < fields_count; i++) {
+		size_t field_start = i * REGMAP_FIELD_SIZE;
+		size_t field_end = field_start + REGMAP_FIELD_SIZE - 1;
+		const struct reg_field field =
+			REG_FIELD(dcfg->offset, field_start, field_end);
+
+		priv->reg_fields[i] =
+			devm_regmap_field_alloc(dev, priv->regmap, field);
+		if (IS_ERR(priv->reg_fields[i]))
+			return PTR_ERR(priv->reg_fields[i]);
+	}
+
 	nvmem = devm_nvmem_register(dev, cfg);
 
 	return PTR_ERR_OR_ZERO(nvmem);
-- 
2.26.3

Re: [PATCH v4 0/4] Bootcount driver using NVMEM cell as backend.

[Nandor Han]

On 5/5/21 1:42 PM, Nandor Han wrote:
> Description
> -----------
> Implement a bootcount (1) related driver that uses
> NVMEM as a backend. The patchset will also update the
> `snvs_lpgpr` driver to support 2 bytes NVMEM cells.
> 
Any feedback about this patchset?

-- 
Regards,
    Nandor

Re: [PATCH v4 2/4] nvmem: bootcount: add bootcount driver

[Srinivas Kandagatla]

On 05/05/2021 11:42, Nandor Han wrote:
> In order to have a robust system we want to be able to identify and take
> actions if a boot loop occurs. This is possible by using the bootcount
> feature, which can be used to identify the number of times device has
> booted since bootcount was last time reset. Bootcount feature (1)
> requires a collaboration between bootloader and user-space, where
> the bootloader will increase a counter and user-space reset it.
> If the counter is not reset and a pre-established threshold is reached,
> bootloader can react and take action.
> 
> This is the kernel side implementation, which can be used to
> identify the number of times device has booted since bootcount was
> last time reset.
> 

If I understand this correctly, this driver is basically exposing a 
nvmem cell via sysfs.

Firstly, This sounds like totally a generic functionality that needs to 
go into nvmem core rather than individual drivers.

Do you see any reason for this not be in core?

Secondly, creating sysfs entries like this in probe will race with 
userspace udev. udev might not notice this new entry in such cases.

Thirdly, You would need to document this in Documentation/ABI/

Finally I noticed that the changes to snvs_lpgpr.c  have not been cced 
to the original author.


--srini

> The driver supports both 16 and 32 bits NVMEM cell size.
> 
> 1) https://www.denx.de/wiki/DULG/UBootBootCountLimit
> 
> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> Signed-off-by: Nandor Han <nandor.han@vaisala.com>
> ---
>   drivers/nvmem/Kconfig           |  10 ++
>   drivers/nvmem/Makefile          |   1 +
>   drivers/nvmem/bootcount-nvmem.c | 195 ++++++++++++++++++++++++++++++++
>   3 files changed, 206 insertions(+)
>   create mode 100644 drivers/nvmem/bootcount-nvmem.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index dd2019006838..d5413c937350 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -288,4 +288,14 @@ config NVMEM_BRCM_NVRAM
>   	  This driver provides support for Broadcom's NVRAM that can be accessed
>   	  using I/O mapping.
>   
> +config BOOTCOUNT_NVMEM
> +	bool "Bootcount driver using nvmem registers"
> +	depends on OF
> +	depends on NVMEM
> +	help
> +	  Driver that implements the bootcount feature support using a
> +	  NVMEM cell as a backend. The driver supports 2 and 4 bytes
> +	  size cells.
> +
> +	  Say y here to enable bootcount support.
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index bbea1410240a..4c77679bbf0d 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
>   nvmem-rmem-y			:= rmem.o
>   obj-$(CONFIG_NVMEM_BRCM_NVRAM)	+= nvmem_brcm_nvram.o
>   nvmem_brcm_nvram-y		:= brcm_nvram.o
> +obj-$(CONFIG_BOOTCOUNT_NVMEM)	+= bootcount-nvmem.o
> diff --git a/drivers/nvmem/bootcount-nvmem.c b/drivers/nvmem/bootcount-nvmem.c
> new file mode 100644
> index 000000000000..7d9b6caefc2b
> --- /dev/null
> +++ b/drivers/nvmem/bootcount-nvmem.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Vaisala Oyj. All rights reserved.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Default magic values from u-boot bootcount drivers */
> +#define BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL16 0xBC00
> +#define BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL32 0xB001C041
> +
> +struct bootcount_nvmem {
> +	struct nvmem_cell *nvmem;
> +	u32 magic;
> +	u32 mask;
> +	size_t bytes_count;
> +};
> +
> +static ssize_t value_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct bootcount_nvmem *bootcount = dev_get_drvdata(dev);
> +	u32 regval;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Check if the value fits */
> +	if ((regval & ~(bootcount->mask)) != 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * In case we use 2 bytes for saving the value we need to take
> +	 * in consideration the endianness of the system. Because of this
> +	 * we mirror the 2 bytes from one side to another.
> +	 * This way, regardless of endianness, the value will be written
> +	 * in the correct order.
> +	 */
> +	if (bootcount->bytes_count == 2) {
> +		regval &= 0xffff;
> +		regval |= (regval & 0xffff) << 16;
> +	}
> +
> +	regval = (~bootcount->mask & bootcount->magic) |
> +		 (regval & bootcount->mask);
> +	ret = nvmem_cell_write(bootcount->nvmem, &regval,
> +			       bootcount->bytes_count);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret != bootcount->bytes_count)
> +		ret = -EIO;
> +	else
> +		ret = count;
> +
> +	return ret;
> +}
> +
> +static ssize_t value_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct bootcount_nvmem *bootcount = dev_get_drvdata(dev);
> +	u32 regval;
> +	void *val;
> +	size_t len;
> +	int ret;
> +
> +	val = nvmem_cell_read(bootcount->nvmem, &len);
> +	if (IS_ERR(val))
> +		return PTR_ERR(val);
> +
> +	if (len != bootcount->bytes_count) {
> +		kfree(val);
> +		return -EINVAL;
> +	}
> +
> +	if (bootcount->bytes_count == 2)
> +		regval = *(u16 *)val;
> +	else
> +		regval = *(u32 *)val;
> +
> +	kfree(val);
> +
> +	if ((regval & ~bootcount->mask) == bootcount->magic)
> +		ret = scnprintf(buf, PAGE_SIZE, "%u\n",
> +				(unsigned int)(regval & bootcount->mask));
> +	else {
> +		dev_warn(dev, "invalid magic value\n");
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RW(value);
> +
> +static int bootcount_nvmem_probe(struct platform_device *pdev)
> +{
> +	struct bootcount_nvmem *bootcount;
> +	int ret;
> +	u32 bits;
> +	void *val = NULL;
> +	size_t len;
> +
> +	bootcount = devm_kzalloc(&pdev->dev, sizeof(struct bootcount_nvmem),
> +				 GFP_KERNEL);
> +	if (!bootcount)
> +		return -ENOMEM;
> +
> +	bootcount->nvmem = devm_nvmem_cell_get(&pdev->dev, "bootcount-regs");
> +	if (IS_ERR(bootcount->nvmem)) {
> +		if (PTR_ERR(bootcount->nvmem) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "cannot get 'bootcount-regs'\n");
> +		return PTR_ERR(bootcount->nvmem);
> +	}
> +
> +	/* detect cell dimensions */
> +	val = nvmem_cell_read(bootcount->nvmem, &len);
> +	if (IS_ERR(val))
> +		return PTR_ERR(val);
> +	kfree(val);
> +	val = NULL;
> +
> +	if (len != 2 && len != 4) {
> +		dev_err(&pdev->dev, "unsupported register size\n");
> +		return -EINVAL;
> +	}
> +
> +	bootcount->bytes_count = len;
> +
> +	platform_set_drvdata(pdev, bootcount);
> +
> +	ret = device_create_file(&pdev->dev, &dev_attr_value);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to export bootcount value\n");
> +		return ret;
> +	}
> +
> +	bits = bootcount->bytes_count << 3;
> +	bootcount->mask = GENMASK((bits >> 1) - 1, 0);
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "linux,bootcount-magic",
> +				   &bootcount->magic);
> +	if (ret == -EINVAL) {
> +		if (bootcount->bytes_count == 2)
> +			bootcount->magic = BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL16;
> +		else
> +			bootcount->magic = BOOTCOUNT_NVMEM_DEFAULT_MAGIC_VAL32;
> +		ret = 0;
> +	} else if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to parse linux,bootcount-magic, error: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	bootcount->magic &= ~bootcount->mask;
> +
> +	return ret;
> +}
> +
> +static int bootcount_nvmem_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_value);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id bootcount_nvmem_match[] = {
> +	{ .compatible = "linux,bootcount-nvmem" },
> +	{},
> +};
> +
> +static struct platform_driver bootcount_nvmem_driver = {
> +	.driver = {
> +		.name = "bootcount-nvmem",
> +		.of_match_table = bootcount_nvmem_match,
> +	},
> +	.probe = bootcount_nvmem_probe,
> +	.remove = bootcount_nvmem_remove,
> +};
> +
> +module_platform_driver(bootcount_nvmem_driver);
> +
> +MODULE_DEVICE_TABLE(of, bootcount_nvmem_match);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Vaisala Oyj");
> +MODULE_DESCRIPTION("Bootcount driver using nvmem compatible registers");
> 

Re: [PATCH v4 2/4] nvmem: bootcount: add bootcount driver

[Nandor Han]

Hi and thanks for your answers.


On 5/28/21 11:23 AM, Srinivas Kandagatla wrote:
> 
> 
> On 05/05/2021 11:42, Nandor Han wrote:
>> In order to have a robust system we want to be able to identify and take
>> actions if a boot loop occurs. This is possible by using the bootcount
>> feature, which can be used to identify the number of times device has
>> booted since bootcount was last time reset. Bootcount feature (1)
>> requires a collaboration between bootloader and user-space, where
>> the bootloader will increase a counter and user-space reset it.
>> If the counter is not reset and a pre-established threshold is reached,
>> bootloader can react and take action.
>>
>> This is the kernel side implementation, which can be used to
>> identify the number of times device has booted since bootcount was
>> last time reset.
>>
> 
> If I understand this correctly, this driver is basically exposing a 
> nvmem cell via sysfs.
> 
> Firstly, This sounds like totally a generic functionality that needs to 
> go into nvmem core rather than individual drivers.
> 
> Do you see any reason for this not be in core?

I agree that exposing a NVMEM cell via sysfs does look as a generic 
functionality. However, the bootcount feature contains also a magic
value that needs to be taken in consideration when extracting the
bootcount value. The size of the field storing the magic and value combo
is configurable as well. The driver will handle this values 
transparentlry for the user and expose only the validated
bootcount value. In case we will only use a generic implementation for
exposing a NVMEM cell via sysfs the aformention functionality will have
to be handled by userspace and this will force the userspace to have
knolwdge about bootcount value format and magic since they will have
to implement it's own functionality about this. In the current solution
the user only have to reset the value to 0 and that's it, the driver
will take care of the rest.

> 
> Secondly, creating sysfs entries like this in probe will race with 
> userspace udev. udev might not notice this new entry in such cases.

Thanks for point this out. I will have a look how to fix this. I'll 
appriciate any advice.

> 
> Thirdly, You would need to document this in Documentation/ABI/
> 

I'll do that.


> Finally I noticed that the changes to snvs_lpgpr.c  have not been cced 
> to the original author.
> 

Sorry, my mistake. I will add it in the next patch-set.
<snip>

-- 
Regards,
    Nandor

Re: [PATCH v4 2/4] nvmem: bootcount: add bootcount driver

[Srinivas Kandagatla]

On 01/06/2021 08:58, Nandor Han wrote:
> Hi and thanks for your answers.
> 
> 
> On 5/28/21 11:23 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 05/05/2021 11:42, Nandor Han wrote:
>>> In order to have a robust system we want to be able to identify and take
>>> actions if a boot loop occurs. This is possible by using the bootcount
>>> feature, which can be used to identify the number of times device has
>>> booted since bootcount was last time reset. Bootcount feature (1)
>>> requires a collaboration between bootloader and user-space, where
>>> the bootloader will increase a counter and user-space reset it.
>>> If the counter is not reset and a pre-established threshold is reached,
>>> bootloader can react and take action.
>>>
>>> This is the kernel side implementation, which can be used to
>>> identify the number of times device has booted since bootcount was
>>> last time reset.
>>>
>>
>> If I understand this correctly, this driver is basically exposing a 
>> nvmem cell via sysfs.
>>
>> Firstly, This sounds like totally a generic functionality that needs 
>> to go into nvmem core rather than individual drivers.
>>
>> Do you see any reason for this not be in core?
> 
> I agree that exposing a NVMEM cell via sysfs does look as a generic 
> functionality. However, the bootcount feature contains also a magic
> value that needs to be taken in consideration when extracting the
> bootcount value. The size of the field storing the magic and value combo
> is configurable as well. The driver will handle this values 
> transparentlry for the user and expose only the validated
> bootcount value. In case we will only use a generic implementation for
> exposing a NVMEM cell via sysfs the aformention functionality will have
> to be handled by userspace and this will force the userspace to have
> knolwdge about bootcount value format and magic since they will have
> to implement it's own functionality about this. In the current solution
> the user only have to reset the value to 0 and that's it, the driver
> will take care of the rest.

Should this not live in userspace HAL, kernel would provide an abstract 
interface. User space in this case which is programming the bootcount is 
already aware of this, so am hoping that it would be able to encapsulate 
the magic as well with in.

Instead of accessing sysfs directly, its always recommended to access it 
via a some abstraction HAL programs, so as to not break the userspace 
across kernel releases, more info at 
./Documentation/admin-guide/sysfs-rules.rst

Other problem with having this in kernel is that we would endup with 
endless number of drivers for each nvmem cell which is totally not 
necessary.

Personally I do not want to endup in such a situation where people start 
writing drivers for each cell.



> 
>>
>> Secondly, creating sysfs entries like this in probe will race with 
>> userspace udev. udev might not notice this new entry in such cases.
> 
> Thanks for point this out. I will have a look how to fix this. I'll 
> appriciate any advice.
> 

There is a good document from Greg KH, 
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/


--srini
>>
>> Thirdly, You would need to document this in Documentation/ABI/
>>
> 
> I'll do that.
> 
> 
>> Finally I noticed that the changes to snvs_lpgpr.c  have not been cced 
>> to the original author.
>>
> 
> Sorry, my mistake. I will add it in the next patch-set.
> <snip>
> 

Re: [PATCH v4 2/4] nvmem: bootcount: add bootcount driver

[Vesa Jääskeläinen]

On 2021-06-03 11:03, Srinivas Kandagatla wrote:
> On 01/06/2021 08:58, Nandor Han wrote:
>> On 5/28/21 11:23 AM, Srinivas Kandagatla wrote:
>>> On 05/05/2021 11:42, Nandor Han wrote:
>>>> In order to have a robust system we want to be able to identify and 
>>>> take
>>>> actions if a boot loop occurs. This is possible by using the bootcount
>>>> feature, which can be used to identify the number of times device has
>>>> booted since bootcount was last time reset. Bootcount feature (1)
>>>> requires a collaboration between bootloader and user-space, where
>>>> the bootloader will increase a counter and user-space reset it.
>>>> If the counter is not reset and a pre-established threshold is reached,
>>>> bootloader can react and take action.
>>>>
>>>> This is the kernel side implementation, which can be used to
>>>> identify the number of times device has booted since bootcount was
>>>> last time reset.
>>>>
>>>
>>> If I understand this correctly, this driver is basically exposing a 
>>> nvmem cell via sysfs.
>>>
>>> Firstly, This sounds like totally a generic functionality that needs 
>>> to go into nvmem core rather than individual drivers.
>>>
>>> Do you see any reason for this not be in core?
>>
>> I agree that exposing a NVMEM cell via sysfs does look as a generic 
>> functionality. However, the bootcount feature contains also a magic
>> value that needs to be taken in consideration when extracting the
>> bootcount value. The size of the field storing the magic and value combo
>> is configurable as well. The driver will handle this values 
>> transparentlry for the user and expose only the validated
>> bootcount value. In case we will only use a generic implementation for
>> exposing a NVMEM cell via sysfs the aformention functionality will have
>> to be handled by userspace and this will force the userspace to have
>> knolwdge about bootcount value format and magic since they will have
>> to implement it's own functionality about this. In the current solution
>> the user only have to reset the value to 0 and that's it, the driver
>> will take care of the rest.
> 
> Should this not live in userspace HAL, kernel would provide an abstract 
> interface. User space in this case which is programming the bootcount is 
> already aware of this, so am hoping that it would be able to encapsulate 
> the magic as well with in.
> 
> Instead of accessing sysfs directly, its always recommended to access it 
> via a some abstraction HAL programs, so as to not break the userspace 
> across kernel releases, more info at 
> ./Documentation/admin-guide/sysfs-rules.rst
> 
> Other problem with having this in kernel is that we would endup with 
> endless number of drivers for each nvmem cell which is totally not 
> necessary.
> 
> Personally I do not want to endup in such a situation where people start 
> writing drivers for each cell.

If we look from U-Boot source code (for which the boot count support has 
been there for long time):

https://source.denx.de/u-boot/u-boot/-/tree/master/drivers/bootcount

In there we do have solutions for:

- Atmel AT91 -- one specific CPU register

- Davinci/Omap/beaglebone -- Uses internal RTC's scratcpad #2 register
   - In here Scratcpad #0/#1 were at least one point of time used for 
deep sleep recovery addresses -- and probably should not be exposed at 
all to user space

- As U-Boot environment variable -- what ever storage would be

- File system interface -- store as a file

- I2C (version 1) -- store in (volatile) 16 bit RTC register

- RAM -- Use multiple addreses for storing magics and actual boot count 
value

- I2C EEPROM -- Store in persistent cells in EEPROM

- RTC -- Store in RTC if the chip has support for it

- SPI flash -- Store in special location in serial flash

So we are already in situation that there exists multiple technical 
solutions :| And best bit here is that some of them can be customized by 
Kconfig options.

Another observation of that list is that those all are not NVMEM cells 
-- so more generic solution abstracting it away would be better. 
Actually the best solution for boot count is probably volatile register 
that persist over reset of the device and is not subject of flash endurance.

So perhaps there should be "core boot count internal API" for which 
driver (in this case nvmem specific) can register itself and then that 
"boot count core" would then expose it to user space. That would most 
likely be quite slim implementation. And most likely there would only be 
one solution per device but in theory it could also support case if 
device supports more than one place to store it then it would handle 
this transparently from the driver.

I believe the sysfs would be perfect for this especially when the path 
for the entry would stay the same independent of the solution behind the 
boot count. This would make it easy for user space to read the boot 
count in shell scripts or in applications and then resetting would be as 
easy as echoing "0" to sysfs entry.

Thanks,
Vesa Jääskeläinen