[PATCH] drivers, char: add U-Boot bootcount driver

[PATCH] drivers, char: add U-Boot bootcount driver

[Heiko Schocher]

This driver implements the Linux kernel half of the boot count feature -
the boot counter can only be reset after it is clear that the
application has been started and is running correctly, which usually
can only be determined by the application code itself. Thus the reset
of the boot counter must be done by application code, which thus needs
an appropriate driver.

Required feature by the Carrier Grade Linux Requirements Definition;
see for example document "Carrier Grade Linux Requirements Definition
Overview V3.0" at

http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection

        Description: OSDL CGL specifies that carrier grade Linux
        shall provide support for detecting a repeating reboot cycle
	due to recurring failures. This detection should happen in
	user space before system services are started.

This driver provides read/write access to the U-Boot bootcounter
through PROC FS and/or sysFS file.

The bootcountregister gets configured via DTS.
for example on the enbw_cmc board:

bootcount@0x23060 {
                  compatible = "uboot,bootcount";
                  reg = <0x23060 0x20>;
                 };

original post from:
http://old.nabble.com/-POWERPC--add-U-Boot-bootcount-driver.-td26804029.html

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Vitaly Bordug <vbordug@ru.mvista.com>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 .../devicetree/bindings/char/bootcount.txt         |   13 ++
 drivers/char/Kconfig                               |    7 +
 drivers/char/Makefile                              |    1 +
 drivers/char/bootcount.c                           |  210 ++++++++++++++++++++
 4 files changed, 231 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/char/bootcount.txt
 create mode 100644 drivers/char/bootcount.c

diff --git a/Documentation/devicetree/bindings/char/bootcount.txt b/Documentation/devicetree/bindings/char/bootcount.txt
new file mode 100644
index 0000000..079a7da
--- /dev/null
+++ b/Documentation/devicetree/bindings/char/bootcount.txt
@@ -0,0 +1,13 @@
+Bootcount driver
+
+Required properties:
+
+  - compatible : should be "uboot,bootcount"
+  - reg: the address of the bootcounter
+
+Example:
+
+bootcount@1c23000 {
+	compatible = "uboot,bootcount";
+	reg = <0x23060 0x20>;
+};
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 4364303..9c5ccab 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -577,6 +577,13 @@ config UV_MMTIMER
 	  The uv_mmtimer device allows direct userspace access to the
 	  UV system timer.
 
+config BOOTCOUNT
+	tristate "U-Boot Bootcount driver"
+	depends on OF
+	help
+	  The U-Boot Bootcount driver allows to access the
+	  bootcounter through procFS or sysFS files.
+
 source "drivers/char/tpm/Kconfig"
 
 config TELCLOCK
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 32762ba..a91e18e 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PC8736x_GPIO)	+= pc8736x_gpio.o
 obj-$(CONFIG_NSC_GPIO)		+= nsc_gpio.o
 obj-$(CONFIG_GPIO_TB0219)	+= tb0219.o
 obj-$(CONFIG_TELCLOCK)		+= tlclk.o
+obj-$(CONFIG_BOOTCOUNT)		+= bootcount.o
 
 obj-$(CONFIG_MWAVE)		+= mwave/
 obj-$(CONFIG_AGP)		+= agp/
diff --git a/drivers/char/bootcount.c b/drivers/char/bootcount.c
new file mode 100644
index 0000000..0ee457f
--- /dev/null
+++ b/drivers/char/bootcount.c
@@ -0,0 +1,210 @@
+/*
+ * This driver gives access(read/write) to the bootcounter used by u-boot.
+ * Access is supported via procFS and sysFS.
+ *
+ * Copyright 2008 DENX Software Engineering GmbH
+ * Author: Heiko Schocher <hs@denx.de>
+ * Based on work from: Steffen Rumler  (Steffen.Rumler@siemens.com)
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/capability.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/ptrace.h>
+#include <linux/uaccess.h>
+
+#ifndef CONFIG_PROC_FS
+#error "PROC FS support must be switched-on"
+#endif
+#include <linux/proc_fs.h>
+
+#define	UBOOT_BOOTCOUNT_MAGIC_OFFSET	0x04	/* offset of magic number */
+#define	UBOOT_BOOTCOUNT_MAGIC		0xB001C041 /* magic number value */
+
+#define	UBOOT_BOOTCOUNT_PROC_ENTRY	"driver/bootcount"
+
+/*
+ * This macro frees the machine specific function from bounds checking and
+ * this like that...
+ */
+#define PRINT_PROC(fmt, args...) \
+	do { \
+		*len += sprintf(buffer + *len, fmt, ##args); \
+		if (*begin + *len > offset + size) \
+			return 0; \
+		if (*begin + *len < offset) { \
+			*begin += *len; \
+			*len = 0; \
+		} \
+	} while (0)
+
+void __iomem *mem;
+/*
+ * read U-Boot bootcounter
+ */
+static int
+read_bootcounter_info(char *buffer, int *len, off_t * begin, off_t offset,
+		       int size)
+{
+	unsigned long magic;
+	unsigned long counter;
+
+
+	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
+	counter = be32_to_cpu(readl(mem));
+
+	if (magic == UBOOT_BOOTCOUNT_MAGIC) {
+		PRINT_PROC("%lu\n", counter);
+	} else {
+		PRINT_PROC("bad magic: 0x%lu != 0x%lu\n", magic,
+			    (unsigned long)UBOOT_BOOTCOUNT_MAGIC);
+	}
+
+	return 1;
+}
+
+/*
+ * read U-Boot bootcounter (wrapper)
+ */
+static int
+read_bootcounter(char *buffer, char **start, off_t offset, int size,
+		  int *eof, void *arg)
+{
+	int len = 0;
+	off_t begin = 0;
+
+
+	*eof = read_bootcounter_info(buffer, &len, &begin, offset, size);
+
+	if (offset >= begin + len)
+		return 0;
+
+	*start = buffer + (offset - begin);
+	return size < begin + len - offset ? size : begin + len - offset;
+}
+
+/*
+ * write new value to U-Boot bootcounter
+ */
+static int
+write_bootcounter(struct file *file, const char *buffer, unsigned long count,
+		   void *data)
+{
+	unsigned long magic;
+
+	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
+	if (magic == UBOOT_BOOTCOUNT_MAGIC)
+		writel(cpu_to_be32(simple_strtol(buffer, NULL, 10)), mem);
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+/* helper for the sysFS */
+static int show_str_bootcount(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	int ret = 0;
+	off_t begin = 0;
+
+	read_bootcounter_info(buf, &ret, &begin, 0, 20);
+	return ret;
+}
+static int store_str_bootcount(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	write_bootcounter(NULL, buf, count, NULL);
+	return count;
+}
+static DEVICE_ATTR(bootcount, S_IWUSR | S_IRUGO, show_str_bootcount,
+		store_str_bootcount);
+
+static int __devinit bootcount_probe(struct platform_device *ofdev)
+{
+	struct device_node *np = of_node_get(ofdev->dev.of_node);
+	struct proc_dir_entry *bootcount;
+
+	mem = of_iomap(np, 0);
+	if (mem == NULL)
+		dev_err(&ofdev->dev, "%s couldnt map register.\n", __func__);
+
+	/* init ProcFS */
+	bootcount = create_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, 0600, NULL);
+	if (bootcount == NULL) {
+		dev_err(&ofdev->dev, "\n%s (%d): cannot create /proc/%s\n",
+			__FILE__, __LINE__, UBOOT_BOOTCOUNT_PROC_ENTRY);
+	} else {
+
+		bootcount->read_proc = read_bootcounter;
+		bootcount->write_proc = write_bootcounter;
+		dev_info(&ofdev->dev, "created \"/proc/%s\"\n",
+			UBOOT_BOOTCOUNT_PROC_ENTRY);
+	}
+
+	if (device_create_file(&ofdev->dev, &dev_attr_bootcount))
+		dev_warn(&ofdev->dev, "%s couldnt register sysFS entry.\n",
+			__func__);
+
+	return 0;
+}
+
+static int __devexit bootcount_remove(struct platform_device *ofdev)
+{
+	BUG();
+	return 0;
+}
+
+static const struct of_device_id __devinitconst bootcount_match[] = {
+	{
+		.compatible = "uboot,bootcount",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, bootcount_match);
+
+static struct platform_driver bootcount_driver = {
+	.driver = {
+		.name = "bootcount",
+		.of_match_table = bootcount_match,
+		.owner = THIS_MODULE,
+	},
+	.probe = bootcount_probe,
+	.remove = bootcount_remove,
+};
+
+
+static int __init uboot_bootcount_init(void)
+{
+	return platform_driver_register(&bootcount_driver);
+}
+
+static void __exit uboot_bootcount_cleanup(void)
+{
+	if (mem != NULL)
+		iounmap(mem);
+	remove_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, NULL);
+}
+
+module_init(uboot_bootcount_init);
+module_exit(uboot_bootcount_cleanup);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Steffen Rumler <steffen.rumler@siemens.com>");
+MODULE_DESCRIPTION("Provide (read/write) access to the U-Boot bootcounter via PROC FS");
-- 
1.7.6.4

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Matthias Kaehlcke]

hi,

some comments inline

El Sun, Dec 04, 2011 at 10:45:21AM +0100 Heiko Schocher ha dit:

> This driver implements the Linux kernel half of the boot count feature -
> the boot counter can only be reset after it is clear that the
> application has been started and is running correctly, which usually
> can only be determined by the application code itself. Thus the reset
> of the boot counter must be done by application code, which thus needs
> an appropriate driver.
> 
> Required feature by the Carrier Grade Linux Requirements Definition;
> see for example document "Carrier Grade Linux Requirements Definition
> Overview V3.0" at
> 
> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
> 
>         Description: OSDL CGL specifies that carrier grade Linux
>         shall provide support for detecting a repeating reboot cycle
> 	due to recurring failures. This detection should happen in
> 	user space before system services are started.
> 
> This driver provides read/write access to the U-Boot bootcounter
> through PROC FS and/or sysFS file.
> 
> The bootcountregister gets configured via DTS.
> for example on the enbw_cmc board:
> 
> bootcount@0x23060 {
>                   compatible = "uboot,bootcount";
>                   reg = <0x23060 0x20>;
>                  };
> 
> original post from:
> http://old.nabble.com/-POWERPC--add-U-Boot-bootcount-driver.-td26804029.html
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Vitaly Bordug <vbordug@ru.mvista.com>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  .../devicetree/bindings/char/bootcount.txt         |   13 ++
>  drivers/char/Kconfig                               |    7 +
>  drivers/char/Makefile                              |    1 +
>  drivers/char/bootcount.c                           |  210 ++++++++++++++++++++
>  4 files changed, 231 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/char/bootcount.txt
>  create mode 100644 drivers/char/bootcount.c
> 
> diff --git a/Documentation/devicetree/bindings/char/bootcount.txt b/Documentation/devicetree/bindings/char/bootcount.txt
> new file mode 100644
> index 0000000..079a7da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/char/bootcount.txt
> @@ -0,0 +1,13 @@
> +Bootcount driver
> +
> +Required properties:
> +
> +  - compatible : should be "uboot,bootcount"
> +  - reg: the address of the bootcounter
> +
> +Example:
> +
> +bootcount@1c23000 {
> +	compatible = "uboot,bootcount";
> +	reg = <0x23060 0x20>;
> +};
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 4364303..9c5ccab 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -577,6 +577,13 @@ config UV_MMTIMER
>  	  The uv_mmtimer device allows direct userspace access to the
>  	  UV system timer.
>  
> +config BOOTCOUNT
> +	tristate "U-Boot Bootcount driver"
> +	depends on OF
> +	help
> +	  The U-Boot Bootcount driver allows to access the
> +	  bootcounter through procFS or sysFS files.
> +
>  source "drivers/char/tpm/Kconfig"
>  
>  config TELCLOCK
> diff --git a/drivers/char/Makefile b/drivers/char/Makefile
> index 32762ba..a91e18e 100644
> --- a/drivers/char/Makefile
> +++ b/drivers/char/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_PC8736x_GPIO)	+= pc8736x_gpio.o
>  obj-$(CONFIG_NSC_GPIO)		+= nsc_gpio.o
>  obj-$(CONFIG_GPIO_TB0219)	+= tb0219.o
>  obj-$(CONFIG_TELCLOCK)		+= tlclk.o
> +obj-$(CONFIG_BOOTCOUNT)		+= bootcount.o
>  
>  obj-$(CONFIG_MWAVE)		+= mwave/
>  obj-$(CONFIG_AGP)		+= agp/
> diff --git a/drivers/char/bootcount.c b/drivers/char/bootcount.c
> new file mode 100644
> index 0000000..0ee457f
> --- /dev/null
> +++ b/drivers/char/bootcount.c
> @@ -0,0 +1,210 @@
> +/*
> + * This driver gives access(read/write) to the bootcounter used by u-boot.
> + * Access is supported via procFS and sysFS.
> + *
> + * Copyright 2008 DENX Software Engineering GmbH
> + * Author: Heiko Schocher <hs@denx.de>
> + * Based on work from: Steffen Rumler  (Steffen.Rumler@siemens.com)
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/capability.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/ptrace.h>
> +#include <linux/uaccess.h>
> +
> +#ifndef CONFIG_PROC_FS
> +#error "PROC FS support must be switched-on"
> +#endif
> +#include <linux/proc_fs.h>
> +
> +#define	UBOOT_BOOTCOUNT_MAGIC_OFFSET	0x04	/* offset of magic number */
> +#define	UBOOT_BOOTCOUNT_MAGIC		0xB001C041 /* magic number value */
> +
> +#define	UBOOT_BOOTCOUNT_PROC_ENTRY	"driver/bootcount"
> +
> +/*
> + * This macro frees the machine specific function from bounds checking and
> + * this like that...
> + */
> +#define PRINT_PROC(fmt, args...) \
> +	do { \
> +		*len += sprintf(buffer + *len, fmt, ##args); \
> +		if (*begin + *len > offset + size) \
> +			return 0; \
> +		if (*begin + *len < offset) { \
> +			*begin += *len; \
> +			*len = 0; \
> +		} \
> +	} while (0)
> +
> +void __iomem *mem;

+static void __iomem *mem;

> +/*
> + * read U-Boot bootcounter
> + */
> +static int
> +read_bootcounter_info(char *buffer, int *len, off_t * begin, off_t offset,
> +		       int size)
> +{
> +	unsigned long magic;
> +	unsigned long counter;
> +
> +

remove second empty line

> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	counter = be32_to_cpu(readl(mem));
> +
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC) {
> +		PRINT_PROC("%lu\n", counter);
> +	} else {
> +		PRINT_PROC("bad magic: 0x%lu != 0x%lu\n", magic,
> +			    (unsigned long)UBOOT_BOOTCOUNT_MAGIC);
> +	}

no need for curly braces

> +
> +	return 1;
> +}
> +
> +/*
> + * read U-Boot bootcounter (wrapper)
> + */
> +static int
> +read_bootcounter(char *buffer, char **start, off_t offset, int size,
> +		  int *eof, void *arg)
> +{
> +	int len = 0;
> +	off_t begin = 0;
> +
> +

remove second empty line

> +	*eof = read_bootcounter_info(buffer, &len, &begin, offset, size);
> +
> +	if (offset >= begin + len)
> +		return 0;
> +
> +	*start = buffer + (offset - begin);
> +	return size < begin + len - offset ? size : begin + len - offset;
> +}
> +
> +/*
> + * write new value to U-Boot bootcounter
> + */
> +static int
> +write_bootcounter(struct file *file, const char *buffer, unsigned long count,
> +		   void *data)
> +{
> +	unsigned long magic;
> +
> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC)
> +		writel(cpu_to_be32(simple_strtol(buffer, NULL, 10)), mem);
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/* helper for the sysFS */
> +static int show_str_bootcount(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	int ret = 0;
> +	off_t begin = 0;
> +
> +	read_bootcounter_info(buf, &ret, &begin, 0, 20);
> +	return ret;
> +}
> +static int store_str_bootcount(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	write_bootcounter(NULL, buf, count, NULL);
> +	return count;
> +}
> +static DEVICE_ATTR(bootcount, S_IWUSR | S_IRUGO, show_str_bootcount,
> +		store_str_bootcount);
> +
> +static int __devinit bootcount_probe(struct platform_device *ofdev)
> +{
> +	struct device_node *np = of_node_get(ofdev->dev.of_node);
> +	struct proc_dir_entry *bootcount;
> +
> +	mem = of_iomap(np, 0);
> +	if (mem == NULL)
> +		dev_err(&ofdev->dev, "%s couldnt map register.\n", __func__);

return -1; ?

also applies to other error paths in this function. carrying on
without the register mapped or without access to the bootcounter
doesn't seem to make much sense

> +
> +	/* init ProcFS */
> +	bootcount = create_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, 0600, NULL);
> +	if (bootcount == NULL) {
> +		dev_err(&ofdev->dev, "\n%s (%d): cannot create /proc/%s\n",
> +			__FILE__, __LINE__, UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	} else {
> +
> +		bootcount->read_proc = read_bootcounter;
> +		bootcount->write_proc = write_bootcounter;
> +		dev_info(&ofdev->dev, "created \"/proc/%s\"\n",
> +			UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	}
> +
> +	if (device_create_file(&ofdev->dev, &dev_attr_bootcount))
> +		dev_warn(&ofdev->dev, "%s couldnt register sysFS entry.\n",
> +			__func__);
> +
> +	return 0;
> +}
> +
> +static int __devexit bootcount_remove(struct platform_device *ofdev)
> +{
> +	BUG();
> +	return 0;
> +}
> +
> +static const struct of_device_id __devinitconst bootcount_match[] = {
> +	{
> +		.compatible = "uboot,bootcount",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bootcount_match);
> +
> +static struct platform_driver bootcount_driver = {
> +	.driver = {
> +		.name = "bootcount",
> +		.of_match_table = bootcount_match,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = bootcount_probe,
> +	.remove = bootcount_remove,
> +};
> +
> +
> +static int __init uboot_bootcount_init(void)
> +{
> +	return platform_driver_register(&bootcount_driver);
> +}
> +
> +static void __exit uboot_bootcount_cleanup(void)
> +{
> +	if (mem != NULL)
> +		iounmap(mem);
> +	remove_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, NULL);
> +}
> +
> +module_init(uboot_bootcount_init);
> +module_exit(uboot_bootcount_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Steffen Rumler <steffen.rumler@siemens.com>");
> +MODULE_DESCRIPTION("Provide (read/write) access to the U-Boot bootcounter via PROC FS");

-- 
Matthias Kaehlcke
Embedded Linux Developer
Amsterdam

          The opposite of love is not hate, it's indifference
                            (Elie Wiesel)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
> This driver implements the Linux kernel half of the boot count feature -
> the boot counter can only be reset after it is clear that the
> application has been started and is running correctly, which usually
> can only be determined by the application code itself. Thus the reset
> of the boot counter must be done by application code, which thus needs
> an appropriate driver.

An appropriate mechanism, not necessarily a driver, see below.

> Required feature by the Carrier Grade Linux Requirements Definition;
> see for example document "Carrier Grade Linux Requirements Definition
> Overview V3.0" at
> 
> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
> 
>       Description: OSDL CGL specifies that carrier grade Linux
>       shall provide support for detecting a repeating reboot cycle
> 	due to recurring failures. This detection should happen in
> 	user space before system services are started.

So, technically, a flag would be enough, not necessarily a counter? Although a
counter probably has more advantages...

> This driver provides read/write access to the U-Boot bootcounter
> through PROC FS and/or sysFS file.

Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?

> The bootcountregister gets configured via DTS.
> for example on the enbw_cmc board:
> 
> bootcount@0x23060 {
>                   compatible = "uboot,bootcount";

No. I assume you are not the vendor of what is at 0x23060, the actual device.
Only the device must be encoded in the compatible-entry which then implies the
bootcount functionality. Also, keep in mind that your solution should be
generic for bootloaders.

>                   reg = <0x23060 0x20>;

I assume that non-volatile memory would qualify as a boot-counter, so those
could be tied to I2C busses etc? reg would not fit then.

I do wonder if it makes more sense to add such functionality to the
watchdog-core to save the additional device (CCed). Needs a second thought,
though...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfgang Denk]

Dear Wolfram,

in message <20111204114741.GA5788@pengutronix.de> you wrote:
> 
> >       Description: OSDL CGL specifies that carrier grade Linux
> >       shall provide support for detecting a repeating reboot cycle
> > 	due to recurring failures. This detection should happen in
> > 	user space before system services are started.
>
> So, technically, a flag would be enough, not necessarily a counter? Although a
> counter probably has more advantages...

The real-life applications we have seen so far all required a counter.
They all required to switch to a recovery mode or other alternative
boot sequence after N failed boot attempts, with N > 1 in all cases.

> >                   reg = <0x23060 0x20>;
>
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

Actually all kind of non-volatile storage can be used - it depends
on specific hardware roperties.  Some SoCs have registers that are
guaranteed not to change theier value during a reset; on other systems
we have storage or NVRAM in RTCs or similar, or we can use SRAM, or
I2C or SPI attached EEPROM, or even storage on SDCard, USB or other
storage devices.

This binding covers the memory type only.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

* Wolfram Sang wrote:
> On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
[...]
> >                   reg = <0x23060 0x20>;
> 
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

This is not really on-topic, but in the device-tree the "reg" property is
used to encode an I2C device's slave address.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Heiko Schocher]

Hello Wolfram,

Wolfram Sang wrote:
> On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
>> This driver implements the Linux kernel half of the boot count feature -
>> the boot counter can only be reset after it is clear that the
>> application has been started and is running correctly, which usually
>> can only be determined by the application code itself. Thus the reset
>> of the boot counter must be done by application code, which thus needs
>> an appropriate driver.
> 
> An appropriate mechanism, not necessarily a driver, see below.
> 
>> Required feature by the Carrier Grade Linux Requirements Definition;
>> see for example document "Carrier Grade Linux Requirements Definition
>> Overview V3.0" at
>>
>> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
>>
>>       Description: OSDL CGL specifies that carrier grade Linux
>>       shall provide support for detecting a repeating reboot cycle
>> 	due to recurring failures. This detection should happen in
>> 	user space before system services are started.
> 
> So, technically, a flag would be enough, not necessarily a counter? Although a
> counter probably has more advantages...
> 
>> This driver provides read/write access to the U-Boot bootcounter
>> through PROC FS and/or sysFS file.
> 
> Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?

I drop the ProcFS support for v2.

>> The bootcountregister gets configured via DTS.
>> for example on the enbw_cmc board:
>>
>> bootcount@0x23060 {
>>                   compatible = "uboot,bootcount";
> 
> No. I assume you are not the vendor of what is at 0x23060, the actual device.
> Only the device must be encoded in the compatible-entry which then implies the
> bootcount functionality. Also, keep in mind that your solution should be
> generic for bootloaders.

So I should call it compatible = "generic, bootcount" ?

>>                   reg = <0x23060 0x20>;
> 
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

Currently, mem only supported, add this to the Documentation and log
message.

> I do wonder if it makes more sense to add such functionality to the
> watchdog-core to save the additional device (CCed). Needs a second thought,
> though...

Thanks!

bye,
heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

Hi Heiko,

> >> This driver provides read/write access to the U-Boot bootcounter
> >> through PROC FS and/or sysFS file.
> > 
> > Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?
> 
> I drop the ProcFS support for v2.

Don't bother. This approach starts from the wrong side.

> 
> >> The bootcountregister gets configured via DTS.
> >> for example on the enbw_cmc board:
> >>
> >> bootcount@0x23060 {
> >>                   compatible = "uboot,bootcount";
> > 
> > No. I assume you are not the vendor of what is at 0x23060, the actual device.
> > Only the device must be encoded in the compatible-entry which then implies the
> > bootcount functionality. Also, keep in mind that your solution should be
> > generic for bootloaders.
> 
> So I should call it compatible = "generic, bootcount" ?

Nope, you should give it the name of the device. Remember that 'compatible' is
no 1:1 replacement for platform_driver-binding. Check
http://devicetree.org/Device_Tree_Usage, especially the sections about the
compatible-property.

bootcount itself is not a device. It is a feature of certain devices. And that
needs to be implemented; possibly generic enough that it can work for register
based, i2c based, and so forth, accesses.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfgang Denk]

Dear Wolfram,

In message <20111206215056.GD14154@pengutronix.de> you wrote:
> 
> bootcount itself is not a device. It is a feature of certain devices. And that
> needs to be implemented; possibly generic enough that it can work for register
> based, i2c based, and so forth, accesses.

If "boot counter" is not a good name for such a device, then what name
would you suggest?

Or do you think a counter (which can be implemented in a number of
different ways, depending on hardware specifics) is not a device?
What would be such a device, then?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"How is this place run - is it an anarchy?"
"No, I wouldn't say so; it is not that well organised..."

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]


> > bootcount itself is not a device. It is a feature of certain devices. And that
> > needs to be implemented; possibly generic enough that it can work for register
> > based, i2c based, and so forth, accesses.
> 
> If "boot counter" is not a good name for such a device, then what name
> would you suggest?

None.

> Or do you think a counter (which can be implemented in a number of
> different ways, depending on hardware specifics) is not a device?

Yes.

> What would be such a device, then?

"maxim,ds1338"

Please have a look at the devicetree.org-wiki-page I just mentioned:

http://devicetree.org/Device_Tree_Usage

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Rob Herring]

On 12/06/2011 04:06 PM, Wolfram Sang wrote:
> 
>>> bootcount itself is not a device. It is a feature of certain devices. And that
>>> needs to be implemented; possibly generic enough that it can work for register
>>> based, i2c based, and so forth, accesses.
>>
>> If "boot counter" is not a good name for such a device, then what name
>> would you suggest?
> 
> None.
> 
>> Or do you think a counter (which can be implemented in a number of
>> different ways, depending on hardware specifics) is not a device?
> 
> Yes.
> 
>> What would be such a device, then?
> 
> "maxim,ds1338"
> 
> Please have a look at the devicetree.org-wiki-page I just mentioned:
> 
> http://devicetree.org/Device_Tree_Usage
> 

Perhaps fs/pstore would be a good choice for the user space interface
(defining a new file bootcount). This can support any arbitrary backing
device although pretty much only ACPI is implemented.

Rob

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Heiko Schocher]

Hello Rob,

Sorry for the late reply ...

Rob Herring wrote:
> On 12/06/2011 04:06 PM, Wolfram Sang wrote:
>>>> bootcount itself is not a device. It is a feature of certain devices. And that
>>>> needs to be implemented; possibly generic enough that it can work for register
>>>> based, i2c based, and so forth, accesses.
>>> If "boot counter" is not a good name for such a device, then what name
>>> would you suggest?
>> None.
>>
>>> Or do you think a counter (which can be implemented in a number of
>>> different ways, depending on hardware specifics) is not a device?
>> Yes.
>>
>>> What would be such a device, then?
>> "maxim,ds1338"
>>
>> Please have a look at the devicetree.org-wiki-page I just mentioned:
>>
>> http://devicetree.org/Device_Tree_Usage
>>
> 
> Perhaps fs/pstore would be a good choice for the user space interface
> (defining a new file bootcount). This can support any arbitrary backing
> device although pretty much only ACPI is implemented.

I tried to use fs/pstore for the bootcount feature, and I can mount
and read the bootcount value from the file, I created. But I could not
write to that file ... I only see the callback from
include/linux/pstore.h

struct pstore_info {
[...]
        int             (*write)(enum pstore_type_id type,
                        enum kmsg_dump_reason reason, u64 *id,
                        unsigned int part, size_t size, struct pstore_info *psi);

called, if I reboot ... do I miss something? Or is it not possible
to write to the files created through fs/pstore?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Paul Bolle]

Heiko,

Some minor comments follow. These should touch things not yet covered by
Matthias Kaehlcke or Wolfram Sang.

On Sun, 2011-12-04 at 10:45 +0100, Heiko Schocher wrote:
>[...]
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 4364303..9c5ccab 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -577,6 +577,13 @@ config UV_MMTIMER
>  	  The uv_mmtimer device allows direct userspace access to the
>  	  UV system timer.
>  
> +config BOOTCOUNT
> +	tristate "U-Boot Bootcount driver"
> +	depends on OF
> +	help
> +	  The U-Boot Bootcount driver allows to access the
> +	  bootcounter through procFS or sysFS files.

This is apparently U-Boot specific. So perhaps you could rename this to
config UBOOT_BOOTCOUNT. That should make it easier to later add config
[...]_BOOTCOUNT for other bootloaders and push all common stuff under
config BOOTCOUNT (a rather generic name).

>  source "drivers/char/tpm/Kconfig"
>  
>  config TELCLOCK
> [...]
> diff --git a/drivers/char/bootcount.c b/drivers/char/bootcount.c
> new file mode 100644
> index 0000000..0ee457f
> --- /dev/null
> +++ b/drivers/char/bootcount.c
> @@ -0,0 +1,210 @@
> +/*
> + * This driver gives access(read/write) to the bootcounter used by u-boot.
> + * Access is supported via procFS and sysFS.
> + *
> + * Copyright 2008 DENX Software Engineering GmbH
> + * Author: Heiko Schocher <hs@denx.de>
> + * Based on work from: Steffen Rumler  (Steffen.Rumler@siemens.com)

Angle brackets for the email address?

> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/capability.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/ptrace.h>
> +#include <linux/uaccess.h>
> +
> +#ifndef CONFIG_PROC_FS
> +#error "PROC FS support must be switched-on"
> +#endif

Why is this error needed? Can't you depend on PROC_FS in the Kconfig
file?

> +#include <linux/proc_fs.h>
> +
> +#define	UBOOT_BOOTCOUNT_MAGIC_OFFSET	0x04	/* offset of magic number */
> +#define	UBOOT_BOOTCOUNT_MAGIC		0xB001C041 /* magic number value */
> +
> +#define	UBOOT_BOOTCOUNT_PROC_ENTRY	"driver/bootcount"
> +
> +/*
> + * This macro frees the machine specific function from bounds checking and
> + * this like that...
> + */
> +#define PRINT_PROC(fmt, args...) \
> +	do { \
> +		*len += sprintf(buffer + *len, fmt, ##args); \
> +		if (*begin + *len > offset + size) \
> +			return 0; \
> +		if (*begin + *len < offset) { \
> +			*begin += *len; \
> +			*len = 0; \
> +		} \
> +	} while (0)
> +
> +void __iomem *mem;

Empty line here.

> +/*
> + * read U-Boot bootcounter
> + */

This comment basically repeats the function name.

> +static int
> +read_bootcounter_info(char *buffer, int *len, off_t * begin, off_t offset,
> +		       int size)
> +{
> +	unsigned long magic;
> +	unsigned long counter;
> +
> +
> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	counter = be32_to_cpu(readl(mem));
> +
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC) {
> +		PRINT_PROC("%lu\n", counter);
> +	} else {
> +		PRINT_PROC("bad magic: 0x%lu != 0x%lu\n", magic,
> +			    (unsigned long)UBOOT_BOOTCOUNT_MAGIC);
> +	}
> +
> +	return 1;
> +}
> +
> +/*
> + * read U-Boot bootcounter (wrapper)
> + */

Ditto.

> +static int
> +read_bootcounter(char *buffer, char **start, off_t offset, int size,
> +		  int *eof, void *arg)
> +{
> +	int len = 0;
> +	off_t begin = 0;
> +
> +
> +	*eof = read_bootcounter_info(buffer, &len, &begin, offset, size);
> +
> +	if (offset >= begin + len)
> +		return 0;
> +
> +	*start = buffer + (offset - begin);
> +	return size < begin + len - offset ? size : begin + len - offset;
> +}
> +
> +/*
> + * write new value to U-Boot bootcounter
> + */
> +static int
> +write_bootcounter(struct file *file, const char *buffer, unsigned long count,
> +		   void *data)
> +{
> +	unsigned long magic;
> +
> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC)
> +		writel(cpu_to_be32(simple_strtol(buffer, NULL, 10)), mem);
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/* helper for the sysFS */
> +static int show_str_bootcount(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	int ret = 0;
> +	off_t begin = 0;
> +
> +	read_bootcounter_info(buf, &ret, &begin, 0, 20);
> +	return ret;
> +}

Empty line here.

> +static int store_str_bootcount(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	write_bootcounter(NULL, buf, count, NULL);
> +	return count;
> +}

Empty line here.

> +static DEVICE_ATTR(bootcount, S_IWUSR | S_IRUGO, show_str_bootcount,
> +		store_str_bootcount);
> +
> +static int __devinit bootcount_probe(struct platform_device *ofdev)
> +{
> +	struct device_node *np = of_node_get(ofdev->dev.of_node);
> +	struct proc_dir_entry *bootcount;
> +
> +	mem = of_iomap(np, 0);
> +	if (mem == NULL)
> +		dev_err(&ofdev->dev, "%s couldnt map register.\n", __func__);
> +
> +	/* init ProcFS */
> +	bootcount = create_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, 0600, NULL);
> +	if (bootcount == NULL) {
> +		dev_err(&ofdev->dev, "\n%s (%d): cannot create /proc/%s\n",
> +			__FILE__, __LINE__, UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	} else {
> +
> +		bootcount->read_proc = read_bootcounter;
> +		bootcount->write_proc = write_bootcounter;
> +		dev_info(&ofdev->dev, "created \"/proc/%s\"\n",
> +			UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	}
> +
> +	if (device_create_file(&ofdev->dev, &dev_attr_bootcount))
> +		dev_warn(&ofdev->dev, "%s couldnt register sysFS entry.\n",
> +			__func__);
> +
> +	return 0;
> +}
> +
> +static int __devexit bootcount_remove(struct platform_device *ofdev)
> +{
> +	BUG();

Why BUG() here? And there's no comment, so someone like me - entirely
unfamiliar with this stuff - who triggers this bug and checks this
location will still not know what went wrong.

> +	return 0;

And can't you return non-zero here? I haven't checked this interface,
but returning zero after BUG() looks odd.

> +}
> +
> +static const struct of_device_id __devinitconst bootcount_match[] = {
> +	{
> +		.compatible = "uboot,bootcount",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bootcount_match);
> +
> +static struct platform_driver bootcount_driver = {
> +	.driver = {
> +		.name = "bootcount",
> +		.of_match_table = bootcount_match,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = bootcount_probe,
> +	.remove = bootcount_remove,

Perhaps .remove can be dropped entirely (again, I haven't checked this
interface). Then the BUG() is unneeded, isn't it?

> +};
> +

Drop newline here.

> +static int __init uboot_bootcount_init(void)
> +{
> +	return platform_driver_register(&bootcount_driver);
> +}
> +
> +static void __exit uboot_bootcount_cleanup(void)
> +{
> +	if (mem != NULL)
> +		iounmap(mem);
> +	remove_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, NULL);
> +}
> +
> +module_init(uboot_bootcount_init);
> +module_exit(uboot_bootcount_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Steffen Rumler <steffen.rumler@siemens.com>");

In the comment at the top you're the author. Steffen is here. And
Steffen is also not mailed this patch directly. Does Steffen wish to be
sent messages regarding this driver in the future?  

> +MODULE_DESCRIPTION("Provide (read/write) access to the U-Boot bootcounter via PROC FS");

What happened to sysfs here?


Paul Bolle

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Ryan Mallon]

On 04/12/11 20:45, Heiko Schocher wrote:

> This driver implements the Linux kernel half of the boot count feature -
> the boot counter can only be reset after it is clear that the
> application has been started and is running correctly, which usually
> can only be determined by the application code itself. Thus the reset
> of the boot counter must be done by application code, which thus needs
> an appropriate driver.
> 
> Required feature by the Carrier Grade Linux Requirements Definition;
> see for example document "Carrier Grade Linux Requirements Definition
> Overview V3.0" at
> 
> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
> 
>         Description: OSDL CGL specifies that carrier grade Linux
>         shall provide support for detecting a repeating reboot cycle
> 	due to recurring failures. This detection should happen in
> 	user space before system services are started.
> 
> This driver provides read/write access to the U-Boot bootcounter
> through PROC FS and/or sysFS file.

You should just support one interface. Sysfs seems much more appropriate
for something like this. You should also add documentation for the sysfs
interface to Documentation/ABI/ for the interface.

> The bootcountregister gets configured via DTS.
> for example on the enbw_cmc board:
> 
> bootcount@0x23060 {
>                   compatible = "uboot,bootcount";
>                   reg = <0x23060 0x20>;
>                  };
> 
> original post from:
> http://old.nabble.com/-POWERPC--add-U-Boot-bootcount-driver.-td26804029.html
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Vitaly Bordug <vbordug@ru.mvista.com>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  .../devicetree/bindings/char/bootcount.txt         |   13 ++
>  drivers/char/Kconfig                               |    7 +
>  drivers/char/Makefile                              |    1 +
>  drivers/char/bootcount.c                           |  210 ++++++++++++++++++++
>  4 files changed, 231 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/char/bootcount.txt
>  create mode 100644 drivers/char/bootcount.c
> 
> diff --git a/Documentation/devicetree/bindings/char/bootcount.txt b/Documentation/devicetree/bindings/char/bootcount.txt
> new file mode 100644
> index 0000000..079a7da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/char/bootcount.txt
> @@ -0,0 +1,13 @@
> +Bootcount driver
> +
> +Required properties:
> +
> +  - compatible : should be "uboot,bootcount"
> +  - reg: the address of the bootcounter
> +
> +Example:
> +
> +bootcount@1c23000 {
> +	compatible = "uboot,bootcount";
> +	reg = <0x23060 0x20>;
> +};
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 4364303..9c5ccab 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -577,6 +577,13 @@ config UV_MMTIMER
>  	  The uv_mmtimer device allows direct userspace access to the
>  	  UV system timer.
>  
> +config BOOTCOUNT


Should be called UBOOT_BOOTCOUNT or similar.

> +	tristate "U-Boot Bootcount driver"
> +	depends on OF


Why does it depend on OF? Can't you pass the required information in
platform_data for non-OF platforms?

> +	help
> +	  The U-Boot Bootcount driver allows to access the
> +	  bootcounter through procFS or sysFS files.
> +
>  source "drivers/char/tpm/Kconfig"
>  
>  config TELCLOCK
> diff --git a/drivers/char/Makefile b/drivers/char/Makefile
> index 32762ba..a91e18e 100644
> --- a/drivers/char/Makefile
> +++ b/drivers/char/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_PC8736x_GPIO)	+= pc8736x_gpio.o
>  obj-$(CONFIG_NSC_GPIO)		+= nsc_gpio.o
>  obj-$(CONFIG_GPIO_TB0219)	+= tb0219.o
>  obj-$(CONFIG_TELCLOCK)		+= tlclk.o
> +obj-$(CONFIG_BOOTCOUNT)		+= bootcount.o

>

>  obj-$(CONFIG_MWAVE)		+= mwave/
>  obj-$(CONFIG_AGP)		+= agp/
> diff --git a/drivers/char/bootcount.c b/drivers/char/bootcount.c


Probably uboot_bootcount is a better name for the file also. Why is this
in drivers char when it isn't a char device? drivers/misc might be a
better place for this. I also wonder if it needs to be a driver at all,
though not sure where the best place to put its sysfs files would be if not.

Someone else suggested hooking onto the existing watchdog subsystem. It
might be a good idea to make a more generic bootcount interface with
this as a user of it.

> new file mode 100644
> index 0000000..0ee457f
> --- /dev/null
> +++ b/drivers/char/bootcount.c
> @@ -0,0 +1,210 @@
> +/*
> + * This driver gives access(read/write) to the bootcounter used by u-boot.
> + * Access is supported via procFS and sysFS.
> + *
> + * Copyright 2008 DENX Software Engineering GmbH
> + * Author: Heiko Schocher <hs@denx.de>
> + * Based on work from: Steffen Rumler  (Steffen.Rumler@siemens.com)
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/capability.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/ptrace.h>
> +#include <linux/uaccess.h>


I don't think you need all of these headers included. Strip out the ones
you aren't using.

> +
> +#ifndef CONFIG_PROC_FS
> +#error "PROC FS support must be switched-on"
> +#endif
> +#include <linux/proc_fs.h>
> +
> +#define	UBOOT_BOOTCOUNT_MAGIC_OFFSET	0x04	/* offset of magic number */
> +#define	UBOOT_BOOTCOUNT_MAGIC		0xB001C041 /* magic number value */
> +
> +#define	UBOOT_BOOTCOUNT_PROC_ENTRY	"driver/bootcount"
> +
> +/*
> + * This macro frees the machine specific function from bounds checking and
> + * this like that...
> + */
> +#define PRINT_PROC(fmt, args...) \
> +	do { \
> +		*len += sprintf(buffer + *len, fmt, ##args); \
> +		if (*begin + *len > offset + size) \
> +			return 0; \
> +		if (*begin + *len < offset) { \
> +			*begin += *len; \
> +			*len = 0; \
> +		} \
> +	} while (0)


Ick. This sort of thing should be written as a function, not a macro.
You are also referring to a variable called len which is not passed in,
and calling return inside a macro. Nasty.

I think the seq_file interface does what you want. However, if you just
stick to sysfs, then you shouldn't need all this magic at all.

> +
> +void __iomem *mem;


Static.

> +/*
> + * read U-Boot bootcounter
> + */


Pointless comment.

> +static int
> +read_bootcounter_info(char *buffer, int *len, off_t * begin, off_t offset,
> +		       int size)


const char *buffer?
What are begin, offset and size for? They don't get used in this function.

> +{
> +	unsigned long magic;
> +	unsigned long counter;
> +
> +


Unnecessary whitespace.

> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	counter = be32_to_cpu(readl(mem));
> +
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC) {
> +		PRINT_PROC("%lu\n", counter);
> +	} else {
> +		PRINT_PROC("bad magic: 0x%lu != 0x%lu\n", magic,
> +			    (unsigned long)UBOOT_BOOTCOUNT_MAGIC);
> +	}


Don't need the braces on the if/else. In the failure case you shouldn't
print anything and instead return a proper error code so that userspace
can detect the failure properly.

> +
> +	return 1;


What does 1 mean?

> +}
> +
> +/*
> + * read U-Boot bootcounter (wrapper)
> + */


Pointless comment.

> +static int
> +read_bootcounter(char *buffer, char **start, off_t offset, int size,
> +		  int *eof, void *arg)
> +{
> +	int len = 0;
> +	off_t begin = 0;


Begin isn't used. It gets passed by reference into read_bootcount_info
which does nothing with it, so it is always zero?

Oh, wait, begin is actually used by the overly magic PRINT_PROC macro.
Fix it.

> +
> +


Unnecessary whitespace.

> +	*eof = read_bootcounter_info(buffer, &len, &begin, offset, size);
> +
> +	if (offset >= begin + len)
> +		return 0;

> +
> +	*start = buffer + (offset - begin);

> +	return size < begin + len - offset ? size : begin + len - offset;


This seems to be a rather complicated way of writing:

	return min(size, begin + len - offset);

?

> +}
> +
> +/*
> + * write new value to U-Boot bootcounter
> + */
> +static int
> +write_bootcounter(struct file *file, const char *buffer, unsigned long count,
> +		   void *data)
> +{
> +	unsigned long magic;
> +
> +	magic = be32_to_cpu(readl(mem + UBOOT_BOOTCOUNT_MAGIC_OFFSET));
> +	if (magic == UBOOT_BOOTCOUNT_MAGIC)
> +		writel(cpu_to_be32(simple_strtol(buffer, NULL, 10)), mem);


You should use strict_strtol, and check its return value.

Does it make sense to write any value to the boot counter or just zero
to clear it?

Also, you should just check the UBOOT_BOOTCOUNT_MAGIC at probe time and
fail the probe if it is not correct rather than checking it everytime
you read or write the boot count.

> +	else
> +		return -EINVAL;


Returning -EINVAL because the boot count magic is incorrect is a bit
misleading.

> +
> +	return count;
> +}
> +
> +/* helper for the sysFS */
> +static int show_str_bootcount(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	int ret = 0;
> +	off_t begin = 0;
> +
> +	read_bootcounter_info(buf, &ret, &begin, 0, 20);


What is 20? Why is ret passed back in an argument rather than as the
return value of read_bootcounter_info?

> +	return ret;
> +}
> +static int store_str_bootcount(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	write_bootcounter(NULL, buf, count, NULL);
> +	return count;
> +}
> +static DEVICE_ATTR(bootcount, S_IWUSR | S_IRUGO, show_str_bootcount,
> +		store_str_bootcount);
> +
> +static int __devinit bootcount_probe(struct platform_device *ofdev)
> +{
> +	struct device_node *np = of_node_get(ofdev->dev.of_node);
> +	struct proc_dir_entry *bootcount;
> +
> +	mem = of_iomap(np, 0);
> +	if (mem == NULL)
> +		dev_err(&ofdev->dev, "%s couldnt map register.\n", __func__);


You don't want to fail the probe here? You don't really need to print
the value of __func__ here.

> +
> +	/* init ProcFS */
> +	bootcount = create_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, 0600, NULL);
> +	if (bootcount == NULL) {
> +		dev_err(&ofdev->dev, "\n%s (%d): cannot create /proc/%s\n",
> +			__FILE__, __LINE__, UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	} else {
> +
> +		bootcount->read_proc = read_bootcounter;
> +		bootcount->write_proc = write_bootcounter;
> +		dev_info(&ofdev->dev, "created \"/proc/%s\"\n",
> +			UBOOT_BOOTCOUNT_PROC_ENTRY);
> +	}
> +
> +	if (device_create_file(&ofdev->dev, &dev_attr_bootcount))
> +		dev_warn(&ofdev->dev, "%s couldnt register sysFS entry.\n",
> +			__func__);


Again, you should fail the probe if the interface cannot be registered
properly, otherwise the driver is largely useless. Get rid of the
__FILE__, __LINE__, and __func__ things since they aren't really
necessary and the dev_info is also mostly just syslog noise.

> +
> +	return 0;
> +}
> +
> +static int __devexit bootcount_remove(struct platform_device *ofdev)
> +{
> +	BUG();
> +	return 0;


Why can't you uninstall this module?

> +}
> +
> +static const struct of_device_id __devinitconst bootcount_match[] = {
> +	{
> +		.compatible = "uboot,bootcount",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bootcount_match);
> +
> +static struct platform_driver bootcount_driver = {
> +	.driver = {
> +		.name = "bootcount",
> +		.of_match_table = bootcount_match,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = bootcount_probe,
> +	.remove = bootcount_remove,
> +};
> +
> +


Remove second blank line.

> +static int __init uboot_bootcount_init(void)
> +{
> +	return platform_driver_register(&bootcount_driver);
> +}
> +
> +static void __exit uboot_bootcount_cleanup(void)
> +{
> +	if (mem != NULL)
> +		iounmap(mem);


Why would mem be NULL?

> +	remove_proc_entry(UBOOT_BOOTCOUNT_PROC_ENTRY, NULL);


Remove the sysfs file?

> +}
> +
> +module_init(uboot_bootcount_init);
> +module_exit(uboot_bootcount_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Steffen Rumler <steffen.rumler@siemens.com>");
> +MODULE_DESCRIPTION("Provide (read/write) access to the U-Boot bootcounter via PROC FS");