Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Jiri Slaby]

On 03/27/2008 02:08 AM, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> initial char driver for otp memory
> (only read supported atm ... needs real examples/docs for write support)
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Nice,
Acked-by: Jiri Slaby <jirislaby@gmail.com>

> diff --git a/drivers/char/bfin-otp.c b/drivers/char/bfin-otp.c
> new file mode 100644
> index 0000000..2edc284
> --- /dev/null
> +++ b/drivers/char/bfin-otp.c
[...]
> +static int __init bfin_otp_init(void)
> +{
> +	int ret;
> +
> +	stampit();
> +
> +	ret = misc_register(&bfin_otp_misc_device);
> +	if (ret) {
> +		pr_init(KERN_ERR PFX "unable to register a misc device\n");
> +		return ret;
> +	}
> +
> +	pr_init(KERN_INFO PFX "initialized\n");

except the fact, that pr_init definition seems to be broken. Its defined __fmt 
is const, so it should reside in .init.rodata (__initconst), not .init.data 
(__initdata).

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Mike Frysinger]

On Wed, Mar 26, 2008 at 6:33 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 03/27/2008 02:08 AM, Bryan Wu wrote:
>  > From: Mike Frysinger <vapier.adi@gmail.com>
> > +static int __init bfin_otp_init(void)
>  > +{
>  > +     int ret;
>  > +
>  > +     stampit();
>  > +
>  > +     ret = misc_register(&bfin_otp_misc_device);
>  > +     if (ret) {
>  > +             pr_init(KERN_ERR PFX "unable to register a misc device\n");
>  > +             return ret;
>  > +     }
>  > +
>  > +     pr_init(KERN_INFO PFX "initialized\n");
>
>  except the fact, that pr_init definition seems to be broken. Its defined __fmt
>  is const, so it should reside in .init.rodata (__initconst), not .init.data
>  (__initdata).

i dont see an __initconst macro anywhere ?

also, it isnt "broken" in the sense that it "doesnt work" ;)
-mike

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Jiri Slaby]

On 03/26/2008 03:31 PM, Mike Frysinger wrote:
> On Wed, Mar 26, 2008 at 6:33 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> On 03/27/2008 02:08 AM, Bryan Wu wrote:
>>  > From: Mike Frysinger <vapier.adi@gmail.com>
>>> +static int __init bfin_otp_init(void)
>>  > +{
>>  > +     int ret;
>>  > +
>>  > +     stampit();
>>  > +
>>  > +     ret = misc_register(&bfin_otp_misc_device);
>>  > +     if (ret) {
>>  > +             pr_init(KERN_ERR PFX "unable to register a misc device\n");
>>  > +             return ret;
>>  > +     }
>>  > +
>>  > +     pr_init(KERN_INFO PFX "initialized\n");
>>
>>  except the fact, that pr_init definition seems to be broken. Its defined __fmt
>>  is const, so it should reside in .init.rodata (__initconst), not .init.data
>>  (__initdata).
> 
> i dont see an __initconst macro anywhere ?

It's in include/linux/init.h as of 2.6.25-rc2.

> also, it isnt "broken" in the sense that it "doesnt work" ;)

Gcc simply doesn't allow that, you likely got 2 sections with init.data, with 
and without readonly bit set, which is wrong, and it seems, that on some systems 
(debian) you won't even compile it.

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Mike Frysinger]

On Wed, Mar 26, 2008 at 2:34 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 03/26/2008 03:31 PM, Mike Frysinger wrote:
>  > On Wed, Mar 26, 2008 at 6:33 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
>  >> On 03/27/2008 02:08 AM, Bryan Wu wrote:
>  >>  > From: Mike Frysinger <vapier.adi@gmail.com>
>  >>> +static int __init bfin_otp_init(void)
>  >>  > +{
>  >>  > +     int ret;
>  >>  > +
>  >>  > +     stampit();
>  >>  > +
>  >>  > +     ret = misc_register(&bfin_otp_misc_device);
>  >>  > +     if (ret) {
>  >>  > +             pr_init(KERN_ERR PFX "unable to register a misc device\n");
>  >>  > +             return ret;
>  >>  > +     }
>  >>  > +
>  >>  > +     pr_init(KERN_INFO PFX "initialized\n");
>  >>
>  >>  except the fact, that pr_init definition seems to be broken. Its defined __fmt
>  >>  is const, so it should reside in .init.rodata (__initconst), not .init.data
>  >>  (__initdata).
>  >
>  > i dont see an __initconst macro anywhere ?
>
>  It's in include/linux/init.h as of 2.6.25-rc2.

driver was developed against ~2.6.21 and that isnt even in 2.6.24,
that's why the drivers dont currently use it

>  > also, it isnt "broken" in the sense that it "doesnt work" ;)
>
>  Gcc simply doesn't allow that, you likely got 2 sections with init.data, with
>  and without readonly bit set, which is wrong, and it seems, that on some systems
>  (debian) you won't even compile it.

gcc-4.1.2/binutils-2.17 doesnt have a problem compiling/linking the
code in question.  and i imagine i would have noticed a build/link
failure long before testing it on actual hardware.

Bryan can make the changes before pushing for 2.6.25 inclusion ...
otherwise they'll just stay that way until 2.6.25 gets released, and
then i'll do it.
-mike

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Sam Ravnborg]

> 
> >  > also, it isnt "broken" in the sense that it "doesnt work" ;)
> >
> >  Gcc simply doesn't allow that, you likely got 2 sections with init.data, with
> >  and without readonly bit set, which is wrong, and it seems, that on some systems
> >  (debian) you won't even compile it.
> 
> gcc-4.1.2/binutils-2.17 doesnt have a problem compiling/linking the
> code in question.  and i imagine i would have noticed a build/link
> failure long before testing it on actual hardware.
target arch dependent. Try with 64 bit powerpc.

	Sam

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Mike Frysinger]

On Wed, Mar 26, 2008 at 3:09 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
>  > >  > also, it isnt "broken" in the sense that it "doesnt work" ;)
>  > >
>  > >  Gcc simply doesn't allow that, you likely got 2 sections with init.data, with
>  > >  and without readonly bit set, which is wrong, and it seems, that on some systems
>  > >  (debian) you won't even compile it.
>  >
>  > gcc-4.1.2/binutils-2.17 doesnt have a problem compiling/linking the
>  > code in question.  and i imagine i would have noticed a build/link
>  > failure long before testing it on actual hardware.
>
>  target arch dependent. Try with 64 bit powerpc.

this code appears in drivers that are for Blackfin-specific pieces of
hardware.  it may fail to build on
some-random-arch-that-isnt-Blackfin, but if it works for Blackfin,
what do the other arches realistically matter ?

yes, the code will get changed as suggested as it is the right thing
to do, it just isnt a real problem today.
-mike

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Sam Ravnborg]

On Wed, Mar 26, 2008 at 03:14:14PM -0400, Mike Frysinger wrote:
> On Wed, Mar 26, 2008 at 3:09 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> >  > >  > also, it isnt "broken" in the sense that it "doesnt work" ;)
> >  > >
> >  > >  Gcc simply doesn't allow that, you likely got 2 sections with init.data, with
> >  > >  and without readonly bit set, which is wrong, and it seems, that on some systems
> >  > >  (debian) you won't even compile it.
> >  >
> >  > gcc-4.1.2/binutils-2.17 doesnt have a problem compiling/linking the
> >  > code in question.  and i imagine i would have noticed a build/link
> >  > failure long before testing it on actual hardware.
> >
> >  target arch dependent. Try with 64 bit powerpc.
> 
> this code appears in drivers that are for Blackfin-specific pieces of
> hardware.  it may fail to build on
> some-random-arch-that-isnt-Blackfin, but if it works for Blackfin,
> what do the other arches realistically matter ?
> 
> yes, the code will get changed as suggested as it is the right thing
> to do, it just isnt a real problem today.
It is a real problem for build monkeys (like me sometimes) if
an allyesconfig suddenly does not build.

If this driver can be build for other than blackfin I do not
know.

	Sam

Re: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Mike Frysinger]

On Wed, Mar 26, 2008 at 5:08 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Wed, Mar 26, 2008 at 03:14:14PM -0400, Mike Frysinger wrote:
>  > On Wed, Mar 26, 2008 at 3:09 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
>  > >  > >  > also, it isnt "broken" in the sense that it "doesnt work" ;)
>  > >  > >
>  > >  > >  Gcc simply doesn't allow that, you likely got 2 sections with init.data, with
>  > >  > >  and without readonly bit set, which is wrong, and it seems, that on some systems
>  > >  > >  (debian) you won't even compile it.
>  > >  >
>  > >  > gcc-4.1.2/binutils-2.17 doesnt have a problem compiling/linking the
>  > >  > code in question.  and i imagine i would have noticed a build/link
>  > >  > failure long before testing it on actual hardware.
>  > >
>  > >  target arch dependent. Try with 64 bit powerpc.
>  >
>  > this code appears in drivers that are for Blackfin-specific pieces of
>  > hardware.  it may fail to build on
>  > some-random-arch-that-isnt-Blackfin, but if it works for Blackfin,
>  > what do the other arches realistically matter ?
>  >
>  > yes, the code will get changed as suggested as it is the right thing
>  > to do, it just isnt a real problem today.
>
>  It is a real problem for build monkeys (like me sometimes) if
>  an allyesconfig suddenly does not build.

it isnt a problem at all considering the next part:

>  If this driver can be build for other than blackfin I do not
>  know.

it's a device driver for hardware on the Blackfin processor itself.
the Kconfig has "depends on BLACKFIN".  it just aint gonna be an issue
:).
-mike

[PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

initial char driver for otp memory
(only read supported atm ... needs real examples/docs for write support)

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/char/Kconfig    |   28 +++++++
 drivers/char/Makefile   |    1 +
 drivers/char/bfin-otp.c |  189 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/bfin-otp.c

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index dd17d06..989dade 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -495,6 +495,34 @@ config BRIQ_PANEL
 
 	  It's safe to say N here.
 
+config BFIN_OTP
+	tristate "Blackfin On-Chip OTP Memory Support"
+	depends on BLACKFIN && (BF52x || BF54x)
+	default y
+	help
+	  If you say Y here, you will get support for a character device
+	  interface into the One Time Programmable memory pages that are
+	  stored on the Blackfin processor.  This will not get you access
+	  to the secure memory pages however.  You will need to write your
+	  own secure code and reader for that.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called bfin-otp.
+
+	  If unsure, it is safe to say Y.
+
+config BFIN_OTP_WRITE_ENABLE
+	bool "Enable writing support of OTP pages"
+	depends on BFIN_OTP
+	default n
+	help
+	  If you say Y here, you will enable support for writing of the
+	  OTP pages.  This is dangerous by nature as you can only program
+	  the pages once, so only enable this option when you actually
+	  need it so as to not inadvertently clobber data.
+
+	  If unsure, say N.
+
 config PRINTER
 	tristate "Parallel printer support"
 	depends on PARPORT
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index e087ec1..06e6754 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_VIOTAPE)		+= viotape.o
 obj-$(CONFIG_HVCS)		+= hvcs.o
 obj-$(CONFIG_SGI_MBCS)		+= mbcs.o
 obj-$(CONFIG_BRIQ_PANEL)	+= briq_panel.o
+obj-$(CONFIG_BFIN_OTP)		+= bfin-otp.o
 
 obj-$(CONFIG_PRINTER)		+= lp.o
 obj-$(CONFIG_TIPAR)		+= tipar.o
diff --git a/drivers/char/bfin-otp.c b/drivers/char/bfin-otp.c
new file mode 100644
index 0000000..2edc284
--- /dev/null
+++ b/drivers/char/bfin-otp.c
@@ -0,0 +1,189 @@
+/*
+ * Blackfin On-Chip OTP Memory Interface
+ *  Supports BF52x/BF54x
+ *
+ * Copyright 2007-2008 Analog Devices Inc.
+ *
+ * Enter bugs at http://blackfin.uclinux.org/
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#include <asm/blackfin.h>
+#include <asm/uaccess.h>
+
+#define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __func__, __LINE__, ## args)
+#define stampit() stamp("here i am")
+#define pr_init(fmt, args...) ({ static const __initdata char __fmt[] = fmt; printk(__fmt, ## args); })
+
+#define DRIVER_NAME "bfin-otp"
+#define PFX DRIVER_NAME ": "
+
+static DEFINE_MUTEX(bfin_otp_lock);
+
+/* OTP Boot ROM functions */
+#define _BOOTROM_OTP_COMMAND           0xEF000018
+#define _BOOTROM_OTP_READ              0xEF00001A
+#define _BOOTROM_OTP_WRITE             0xEF00001C
+
+static u32 (* const otp_command)(u32 command, u32 value) = (void *)_BOOTROM_OTP_COMMAND;
+static u32 (* const otp_read)(u32 page, u32 flags, u64 *page_content) = (void *)_BOOTROM_OTP_READ;
+static u32 (* const otp_write)(u32 page, u32 flags, u64 *page_content) = (void *)_BOOTROM_OTP_WRITE;
+
+/* otp_command(): defines for "command" */
+#define OTP_INIT             0x00000001
+#define OTP_CLOSE            0x00000002
+
+/* otp_{read,write}(): defines for "flags" */
+#define OTP_LOWER_HALF       0x00000000 /* select upper/lower 64-bit half (bit 0) */
+#define OTP_UPPER_HALF       0x00000001
+#define OTP_NO_ECC           0x00000010 /* do not use ECC */
+#define OTP_LOCK             0x00000020 /* sets page protection bit for page */
+#define OTP_ACCESS_READ      0x00001000
+#define OTP_ACCESS_READWRITE 0x00002000
+
+/* Return values for all functions */
+#define OTP_SUCCESS          0x00000000
+#define OTP_MASTER_ERROR     0x001
+#define OTP_WRITE_ERROR      0x003
+#define OTP_READ_ERROR       0x005
+#define OTP_ACC_VIO_ERROR    0x009
+#define OTP_DATA_MULT_ERROR  0x011
+#define OTP_ECC_MULT_ERROR   0x021
+#define OTP_PREV_WR_ERROR    0x041
+#define OTP_DATA_SB_WARN     0x100
+#define OTP_ECC_SB_WARN      0x200
+
+/**
+ *	bfin_otp_read - Read OTP pages
+ *
+ *	All reads must be in half page chunks (half page == 64 bits).
+ */
+static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count, loff_t *pos)
+{
+	ssize_t bytes_done;
+	u32 page, flags, ret;
+	u64 content;
+
+	stampit();
+
+	if (count % sizeof(u64))
+		return -EMSGSIZE;
+
+	if (mutex_lock_interruptible(&bfin_otp_lock))
+		return -ERESTARTSYS;
+
+	bytes_done = 0;
+	page = *pos / (sizeof(u64) * 2);
+	while (bytes_done < count) {
+		flags = (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
+		stamp("processing page %i (%s)", page, (flags == OTP_UPPER_HALF ? "upper" : "lower"));
+		ret = otp_read(page, flags, &content);
+		if (ret & OTP_MASTER_ERROR) {
+			bytes_done = -EIO;
+			break;
+		}
+		if (copy_to_user(buff + bytes_done, &content, sizeof(content))) {
+			bytes_done = -EFAULT;
+			break;
+		}
+		if (flags == OTP_UPPER_HALF)
+			++page;
+		bytes_done += sizeof(content);
+		*pos += sizeof(content);
+	}
+
+	mutex_unlock(&bfin_otp_lock);
+
+	return bytes_done;
+}
+
+#ifdef CONFIG_BFIN_OTP_WRITE_ENABLE
+/**
+ *	bfin_otp_write - Write OTP pages
+ *
+ *	All writes must be in half page chunks (half page == 64 bits).
+ */
+static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t count, loff_t *pos)
+{
+	stampit();
+
+	if (count % sizeof(u64))
+		return -EMSGSIZE;
+
+	if (mutex_lock_interruptible(&bfin_otp_lock))
+		return -ERESTARTSYS;
+
+	/* need otp_init() documentation before this can be implemented */
+
+	mutex_unlock(&bfin_otp_lock);
+
+	return -EINVAL;
+}
+#else
+# define bfin_otp_write NULL
+#endif
+
+static struct file_operations bfin_otp_fops = {
+	.owner    = THIS_MODULE,
+	.read     = bfin_otp_read,
+	.write    = bfin_otp_write,
+};
+
+static struct miscdevice bfin_otp_misc_device = {
+	.minor    = MISC_DYNAMIC_MINOR,
+	.name     = DRIVER_NAME,
+	.fops     = &bfin_otp_fops,
+};
+
+/**
+ *	bfin_otp_init - Initialize module
+ *
+ *	Registers the device and notifier handler. Actual device
+ *	initialization is handled by bfin_otp_open().
+ */
+static int __init bfin_otp_init(void)
+{
+	int ret;
+
+	stampit();
+
+	ret = misc_register(&bfin_otp_misc_device);
+	if (ret) {
+		pr_init(KERN_ERR PFX "unable to register a misc device\n");
+		return ret;
+	}
+
+	pr_init(KERN_INFO PFX "initialized\n");
+
+	return 0;
+}
+
+/**
+ *	bfin_otp_exit - Deinitialize module
+ *
+ *	Unregisters the device and notifier handler. Actual device
+ *	deinitialization is handled by bfin_otp_close().
+ */
+static void __exit bfin_otp_exit(void)
+{
+	stampit();
+
+	misc_deregister(&bfin_otp_misc_device);
+}
+
+module_init(bfin_otp_init);
+module_exit(bfin_otp_exit);
+
+MODULE_AUTHOR("Mike Frysinger <vapier@gentoo.org>");
+MODULE_DESCRIPTION("Blackfin OTP Memory Interface");
+MODULE_LICENSE("GPL");
-- 
1.5.4.3