[PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Bryan Wu]

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

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/char/bfin-otp.c |  166 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 126 insertions(+), 40 deletions(-)

diff --git a/drivers/char/bfin-otp.c b/drivers/char/bfin-otp.c
index 0a01329..ab30147 100644
--- a/drivers/char/bfin-otp.c
+++ b/drivers/char/bfin-otp.c
@@ -1,6 +1,5 @@
 /*
  * Blackfin On-Chip OTP Memory Interface
- *  Supports BF52x/BF54x
  *
  * Copyright 2007-2008 Analog Devices Inc.
  *
@@ -17,8 +16,10 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <mtd/mtd-abi.h>
 
 #include <asm/blackfin.h>
+#include <asm/bfrom.h>
 #include <asm/uaccess.h>
 
 #define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __func__, __LINE__, ## args)
@@ -30,39 +31,6 @@
 
 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
  *
@@ -86,9 +54,11 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 	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);
+		stamp("processing page %i (0x%x:%s)", page, flags,
+			(flags & OTP_UPPER_HALF ? "upper" : "lower"));
+		ret = bfrom_OtpRead(page, flags, &content);
 		if (ret & OTP_MASTER_ERROR) {
+			stamp("error from otp: 0x%x", ret);
 			bytes_done = -EIO;
 			break;
 		}
@@ -96,7 +66,7 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 			bytes_done = -EFAULT;
 			break;
 		}
-		if (flags == OTP_UPPER_HALF)
+		if (flags & OTP_UPPER_HALF)
 			++page;
 		bytes_done += sizeof(content);
 		*pos += sizeof(content);
@@ -108,14 +78,53 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 }
 
 #ifdef CONFIG_BFIN_OTP_WRITE_ENABLE
+static bool allow_writes;
+
+/**
+ *	bfin_otp_init_timing - setup OTP timing parameters
+ *
+ *	Required before doing any write operation.  Algorithms from HRM.
+ */
+static u32 bfin_otp_init_timing(void)
+{
+	u32 tp1, tp2, tp3, timing;
+
+	tp1 = get_sclk() / 1000000;
+	tp2 = (2 * get_sclk() / 10000000) << 8;
+	tp3 = (0x1401) << 15;
+	timing = tp1 | tp2 | tp3;
+	if (bfrom_OtpCommand(OTP_INIT, timing))
+		return 0;
+
+	return timing;
+}
+
+/**
+ *	bfin_otp_deinit_timing - set timings to only allow reads
+ *
+ *	Should be called after all writes are done.
+ */
+static void bfin_otp_deinit_timing(u32 timing)
+{
+	/* mask bits [31:15] so that any attempts to write fail */
+	bfrom_OtpCommand(OTP_CLOSE, 0);
+	bfrom_OtpCommand(OTP_INIT, timing & ~(-1 << 15));
+	bfrom_OtpCommand(OTP_CLOSE, 0);
+}
+
 /**
- *	bfin_otp_write - Write OTP pages
+ *	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();
+	ssize_t bytes_done;
+	u32 timing, page, base_flags, flags, ret;
+	u64 content;
+
+	if (!allow_writes)
+		return -EACCES;
 
 	if (count % sizeof(u64))
 		return -EMSGSIZE;
@@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
 	if (mutex_lock_interruptible(&bfin_otp_lock))
 		return -ERESTARTSYS;
 
-	/* need otp_init() documentation before this can be implemented */
+	stampit();
+
+	timing = bfin_otp_init_timing();
+	if (timing == 0) {
+		mutex_unlock(&bfin_otp_lock);
+		return -EIO;
+	}
+
+	base_flags = OTP_CHECK_FOR_PREV_WRITE;
+
+	bytes_done = 0;
+	page = *pos / (sizeof(u64) * 2);
+	while (bytes_done < count) {
+		flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
+		stamp("processing page %i (0x%x:%s) from %p", page, flags,
+			(flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
+		if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
+			bytes_done = -EFAULT;
+			break;
+		}
+		ret = bfrom_OtpWrite(page, flags, &content);
+		if (ret & OTP_MASTER_ERROR) {
+			stamp("error from otp: 0x%x", ret);
+			bytes_done = -EIO;
+			break;
+		}
+		if (flags & OTP_UPPER_HALF)
+			++page;
+		bytes_done += sizeof(content);
+		*pos += sizeof(content);
+	}
+
+	bfin_otp_deinit_timing(timing);
 
 	mutex_unlock(&bfin_otp_lock);
 
+	return bytes_done;
+}
+
+static int bfin_otp_ioctl(struct inode *inode, struct file *filp,
+                          unsigned cmd, unsigned long arg)
+{
+	stampit();
+
+	switch (cmd) {
+	case OTPLOCK: {
+		u32 timing;
+		int ret = -EIO;
+
+		if (!allow_writes)
+			return -EACCES;
+
+		if (mutex_lock_interruptible(&bfin_otp_lock))
+			return -ERESTARTSYS;
+
+		timing = bfin_otp_init_timing();
+		if (timing) {
+			u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
+			stamp("locking page %i resulted in 0x%x", arg, otp_result);
+			if (!(otp_result & OTP_MASTER_ERROR))
+				ret = 0;
+
+			bfin_otp_deinit_timing(timing);
+		}
+
+		mutex_unlock(&bfin_otp_lock);
+
+		return ret;
+	}
+
+	case MEMLOCK:
+		allow_writes = false;
+		return 0;
+
+	case MEMUNLOCK:
+		allow_writes = true;
+		return 0;
+	}
+
 	return -EINVAL;
 }
 #else
 # define bfin_otp_write NULL
+# define bfin_otp_ioctl NULL
 #endif
 
 static struct file_operations bfin_otp_fops = {
 	.owner    = THIS_MODULE,
+	.ioctl    = bfin_otp_ioctl,
 	.read     = bfin_otp_read,
 	.write    = bfin_otp_write,
 };
-- 
1.5.6

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Jiri Slaby]

On 10/13/2008 11:13 AM, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>

Some changelog please.

> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
[...]
> @@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
>  	if (mutex_lock_interruptible(&bfin_otp_lock))
>  		return -ERESTARTSYS;
>  
> -	/* need otp_init() documentation before this can be implemented */
> +	stampit();
> +
> +	timing = bfin_otp_init_timing();
> +	if (timing == 0) {
> +		mutex_unlock(&bfin_otp_lock);
> +		return -EIO;
> +	}
> +
> +	base_flags = OTP_CHECK_FOR_PREV_WRITE;
> +
> +	bytes_done = 0;
> +	page = *pos / (sizeof(u64) * 2);
> +	while (bytes_done < count) {
> +		flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
> +		stamp("processing page %i (0x%x:%s) from %p", page, flags,
> +			(flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
> +		if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
> +			bytes_done = -EFAULT;
> +			break;
> +		}
> +		ret = bfrom_OtpWrite(page, flags, &content);
> +		if (ret & OTP_MASTER_ERROR) {
> +			stamp("error from otp: 0x%x", ret);
> +			bytes_done = -EIO;
> +			break;
> +		}
> +		if (flags & OTP_UPPER_HALF)
> +			++page;
> +		bytes_done += sizeof(content);
> +		*pos += sizeof(content);

What happens to pos if it fails later?

> +	}
> +
> +	bfin_otp_deinit_timing(timing);
>  
>  	mutex_unlock(&bfin_otp_lock);
>  
> +	return bytes_done;
> +}
> +
> +static int bfin_otp_ioctl(struct inode *inode, struct file *filp,
> +                          unsigned cmd, unsigned long arg)
> +{
> +	stampit();
> +
> +	switch (cmd) {
> +	case OTPLOCK: {
> +		u32 timing;
> +		int ret = -EIO;
> +
> +		if (!allow_writes)
> +			return -EACCES;
> +
> +		if (mutex_lock_interruptible(&bfin_otp_lock))
> +			return -ERESTARTSYS;
> +
> +		timing = bfin_otp_init_timing();
> +		if (timing) {
> +			u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
> +			stamp("locking page %i resulted in 0x%x", arg, otp_result);
> +			if (!(otp_result & OTP_MASTER_ERROR))
> +				ret = 0;
> +
> +			bfin_otp_deinit_timing(timing);
> +		}
> +
> +		mutex_unlock(&bfin_otp_lock);
> +
> +		return ret;
> +	}
> +
> +	case MEMLOCK:
> +		allow_writes = false;
> +		return 0;
> +
> +	case MEMUNLOCK:
> +		allow_writes = true;
> +		return 0;

Please switch to unlocked_ioctl. It should be pretty easy. You should change
(and check) allow_writes under the mutex anyway.

> +	}
> +
>  	return -EINVAL;

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Mike Frysinger]

On Mon, Oct 13, 2008 at 05:37, Jiri Slaby wrote:
> On 10/13/2008 11:13 AM, Bryan Wu wrote:
>> From: Mike Frysinger <vapier.adi@gmail.com>
>
> Some changelog please.

i implemented write support.  like the subject says ... not sure what
else to say.

>> @@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
>>       if (mutex_lock_interruptible(&bfin_otp_lock))
>>               return -ERESTARTSYS;
>>
>> -     /* need otp_init() documentation before this can be implemented */
>> +     stampit();
>> +
>> +     timing = bfin_otp_init_timing();
>> +     if (timing == 0) {
>> +             mutex_unlock(&bfin_otp_lock);
>> +             return -EIO;
>> +     }
>> +
>> +     base_flags = OTP_CHECK_FOR_PREV_WRITE;
>> +
>> +     bytes_done = 0;
>> +     page = *pos / (sizeof(u64) * 2);
>> +     while (bytes_done < count) {
>> +             flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
>> +             stamp("processing page %i (0x%x:%s) from %p", page, flags,
>> +                     (flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
>> +             if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
>> +                     bytes_done = -EFAULT;
>> +                     break;
>> +             }
>> +             ret = bfrom_OtpWrite(page, flags, &content);
>> +             if (ret & OTP_MASTER_ERROR) {
>> +                     stamp("error from otp: 0x%x", ret);
>> +                     bytes_done = -EIO;
>> +                     break;
>> +             }
>> +             if (flags & OTP_UPPER_HALF)
>> +                     ++page;
>> +             bytes_done += sizeof(content);
>> +             *pos += sizeof(content);
>
> What happens to pos if it fails later?

there is no state maintained in the hardware.  the pos gets updated
only when a half-page actually gets processed.  so there is no
"later".

>> +static int bfin_otp_ioctl(struct inode *inode, struct file *filp,
>> +                          unsigned cmd, unsigned long arg)
>> +{
>> +     stampit();
>> +
>> +     switch (cmd) {
>> +     case OTPLOCK: {
>> +             u32 timing;
>> +             int ret = -EIO;
>> +
>> +             if (!allow_writes)
>> +                     return -EACCES;
>> +
>> +             if (mutex_lock_interruptible(&bfin_otp_lock))
>> +                     return -ERESTARTSYS;
>> +
>> +             timing = bfin_otp_init_timing();
>> +             if (timing) {
>> +                     u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
>> +                     stamp("locking page %i resulted in 0x%x", arg, otp_result);
>> +                     if (!(otp_result & OTP_MASTER_ERROR))
>> +                             ret = 0;
>> +
>> +                     bfin_otp_deinit_timing(timing);
>> +             }
>> +
>> +             mutex_unlock(&bfin_otp_lock);
>> +
>> +             return ret;
>> +     }
>> +
>> +     case MEMLOCK:
>> +             allow_writes = false;
>> +             return 0;
>> +
>> +     case MEMUNLOCK:
>> +             allow_writes = true;
>> +             return 0;
>
> Please switch to unlocked_ioctl. It should be pretty easy.

will do, thanks

> You should change (and check) allow_writes under the mutex anyway.

not really.  the mutex is to restrict access to the OTP hardware, not
driver state.  because there is none.  access to allow_writes is
atomic in the hardware anyways.

thanks for the review
-mike

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Jiri Slaby]

On 10/13/2008 11:43 AM, Mike Frysinger wrote:
> On Mon, Oct 13, 2008 at 05:37, Jiri Slaby wrote:
>> On 10/13/2008 11:13 AM, Bryan Wu wrote:
>>> @@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
>>>       if (mutex_lock_interruptible(&bfin_otp_lock))
>>>               return -ERESTARTSYS;
>>>
>>> -     /* need otp_init() documentation before this can be implemented */
>>> +     stampit();
>>> +
>>> +     timing = bfin_otp_init_timing();
>>> +     if (timing == 0) {
>>> +             mutex_unlock(&bfin_otp_lock);
>>> +             return -EIO;
>>> +     }
>>> +
>>> +     base_flags = OTP_CHECK_FOR_PREV_WRITE;
>>> +
>>> +     bytes_done = 0;
>>> +     page = *pos / (sizeof(u64) * 2);
>>> +     while (bytes_done < count) {
>>> +             flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
>>> +             stamp("processing page %i (0x%x:%s) from %p", page, flags,
>>> +                     (flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
>>> +             if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
>>> +                     bytes_done = -EFAULT;
>>> +                     break;
>>> +             }
>>> +             ret = bfrom_OtpWrite(page, flags, &content);
>>> +             if (ret & OTP_MASTER_ERROR) {
>>> +                     stamp("error from otp: 0x%x", ret);
>>> +                     bytes_done = -EIO;
>>> +                     break;
>>> +             }
>>> +             if (flags & OTP_UPPER_HALF)
>>> +                     ++page;
>>> +             bytes_done += sizeof(content);
>>> +             *pos += sizeof(content);
>> What happens to pos if it fails later?
> 
> there is no state maintained in the hardware.  the pos gets updated
> only when a half-page actually gets processed.  so there is no
> "later".

Sure there is. Next iteration of the loop. I.e. what happens if bfrom_OtpWrite
fails for the second time?

>> You should change (and check) allow_writes under the mutex anyway.
> 
> not really.  the mutex is to restrict access to the OTP hardware, not
> driver state.  because there is none.  access to allow_writes is
> atomic in the hardware anyways.

Yeah, the assignment/check is.

But is this OK to you:
PROCESS 1			PROCESS 2
lock
  set allow_writes
write
   check allow_writes
   be interrupted
				whatever
				unlock
				    unset allow_writes
				sleep
   mutex lock
   the processing...

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Mike Frysinger]

On Mon, Oct 13, 2008 at 05:59, Jiri Slaby wrote:
> On 10/13/2008 11:43 AM, Mike Frysinger wrote:
>> On Mon, Oct 13, 2008 at 05:37, Jiri Slaby wrote:
>>> On 10/13/2008 11:13 AM, Bryan Wu wrote:
>>>> @@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
>>>>       if (mutex_lock_interruptible(&bfin_otp_lock))
>>>>               return -ERESTARTSYS;
>>>>
>>>> -     /* need otp_init() documentation before this can be implemented */
>>>> +     stampit();
>>>> +
>>>> +     timing = bfin_otp_init_timing();
>>>> +     if (timing == 0) {
>>>> +             mutex_unlock(&bfin_otp_lock);
>>>> +             return -EIO;
>>>> +     }
>>>> +
>>>> +     base_flags = OTP_CHECK_FOR_PREV_WRITE;
>>>> +
>>>> +     bytes_done = 0;
>>>> +     page = *pos / (sizeof(u64) * 2);
>>>> +     while (bytes_done < count) {
>>>> +             flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
>>>> +             stamp("processing page %i (0x%x:%s) from %p", page, flags,
>>>> +                     (flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
>>>> +             if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
>>>> +                     bytes_done = -EFAULT;
>>>> +                     break;
>>>> +             }
>>>> +             ret = bfrom_OtpWrite(page, flags, &content);
>>>> +             if (ret & OTP_MASTER_ERROR) {
>>>> +                     stamp("error from otp: 0x%x", ret);
>>>> +                     bytes_done = -EIO;
>>>> +                     break;
>>>> +             }
>>>> +             if (flags & OTP_UPPER_HALF)
>>>> +                     ++page;
>>>> +             bytes_done += sizeof(content);
>>>> +             *pos += sizeof(content);
>>>
>>> What happens to pos if it fails later?
>>
>> there is no state maintained in the hardware.  the pos gets updated
>> only when a half-page actually gets processed.  so there is no
>> "later".
>
> Sure there is. Next iteration of the loop. I.e. what happens if bfrom_OtpWrite
> fails for the second time?

the pos gets updated every time a half-page gets processed.  so if you
call write() and tell it to write 128 bytes, but you get an error half
way through, the pos points right at the place where the error
occurred.  i dont get what you're asking.

>>> You should change (and check) allow_writes under the mutex anyway.
>>
>> not really.  the mutex is to restrict access to the OTP hardware, not
>> driver state.  because there is none.  access to allow_writes is
>> atomic in the hardware anyways.
>
> Yeah, the assignment/check is.
>
> But is this OK to you:
> PROCESS 1                       PROCESS 2
> lock
>  set allow_writes
> write
>   check allow_writes
>   be interrupted
>                                whatever
>                                unlock
>                                    unset allow_writes
>                                sleep
>   mutex lock
>   the processing...

i dont see a problem here.  there is no loss of data, hardware
failure, software crashes, etc...  in other words, there is no
misbehavior.
-mike

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Jiri Slaby]

On 10/13/2008 12:07 PM, Mike Frysinger wrote:
> the pos gets updated every time a half-page gets processed.  so if you
> call write() and tell it to write 128 bytes, but you get an error half
> way through, the pos points right at the place where the error
> occurred.  i dont get what you're asking.

Ah, OK, that's because I don't know exactly what should happen if a write fails.
I though userspace expects the state of the fd to not be touched.

>> But is this OK to you:
>> PROCESS 1                       PROCESS 2
>> lock
>>  set allow_writes
>> write
>>   check allow_writes
>>   be interrupted
>>                                whatever
>>                                unlock
>>                                    unset allow_writes
>>                                sleep
>>   mutex lock
>>   the processing...
> 
> i dont see a problem here.  there is no loss of data, hardware
> failure, software crashes, etc...  in other words, there is no
> misbehavior.

I see no purpose of allow_writes then. Why is it there? I don't need to call
memlock if anybody else did and I raced with him. Also when somebody else
unlocks after finishing of writes I can start failing in the middle of my writes
-- this doesn't have anything to do with locking, but with the design, the one
global variable.

Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Mike Frysinger]

On Mon, Oct 13, 2008 at 06:35, Jiri Slaby wrote:
> On 10/13/2008 12:07 PM, Mike Frysinger wrote:
>>> But is this OK to you:
>>> PROCESS 1                       PROCESS 2
>>> lock
>>>  set allow_writes
>>> write
>>>   check allow_writes
>>>   be interrupted
>>>                                whatever
>>>                                unlock
>>>                                    unset allow_writes
>>>                                sleep
>>>   mutex lock
>>>   the processing...
>>
>> i dont see a problem here.  there is no loss of data, hardware
>> failure, software crashes, etc...  in other words, there is no
>> misbehavior.
>
> I see no purpose of allow_writes then. Why is it there? I don't need to call
> memlock if anybody else did and I raced with him. Also when somebody else
> unlocks after finishing of writes I can start failing in the middle of my writes
> -- this doesn't have anything to do with locking, but with the design, the one
> global variable.

the write bit is per-device, not per-process. so the fact you've
narrowed down a race condition to two instructions doesnt matter, the
behavior is the same.  one process can unlock the hardware while
another process (which did not unlock) now has access.  or vice versa.
 the lock is to prevent in inadvertent writes.  considering such
inadvertent writes could make the processor unbootable, it's a simple
safety measure which is quite common when dealing with storage
technology like this (just look at some of the other drivers in the
char subdir).  an application that wants to write must first enable
write access, do the write, and then disable write access.  the
scenario you describe isnt a realistic use case, so no point in
accounting for it.  OTP writes are very rare outside of development if
they occur at all (which is why there's a Kconfig option to just
disable it completely).
-mike

[PATCH v2] bfin-otp: add writing support

[Mike Frysinger]

The on-chip OTP may be written at runtime, so enable support for it in the
driver.  However, since writing should really be done only on development
systems, don't bend over backwards to make sure the simple software lock
is per-fd -- per-device is OK.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
v2
	- switch to unlocked ioctl as pointed out by Jiri Slaby

 drivers/char/bfin-otp.c |  173 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 129 insertions(+), 44 deletions(-)

diff --git a/drivers/char/bfin-otp.c b/drivers/char/bfin-otp.c
index 0a01329..e3dd24b 100644
--- a/drivers/char/bfin-otp.c
+++ b/drivers/char/bfin-otp.c
@@ -1,8 +1,7 @@
 /*
  * Blackfin On-Chip OTP Memory Interface
- *  Supports BF52x/BF54x
  *
- * Copyright 2007-2008 Analog Devices Inc.
+ * Copyright 2007-2009 Analog Devices Inc.
  *
  * Enter bugs at http://blackfin.uclinux.org/
  *
@@ -17,8 +16,10 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <mtd/mtd-abi.h>
 
 #include <asm/blackfin.h>
+#include <asm/bfrom.h>
 #include <asm/uaccess.h>
 
 #define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __func__, __LINE__, ## args)
@@ -30,39 +31,6 @@
 
 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
  *
@@ -86,9 +54,11 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 	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);
+		stamp("processing page %i (0x%x:%s)", page, flags,
+			(flags & OTP_UPPER_HALF ? "upper" : "lower"));
+		ret = bfrom_OtpRead(page, flags, &content);
 		if (ret & OTP_MASTER_ERROR) {
+			stamp("error from otp: 0x%x", ret);
 			bytes_done = -EIO;
 			break;
 		}
@@ -96,7 +66,7 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 			bytes_done = -EFAULT;
 			break;
 		}
-		if (flags == OTP_UPPER_HALF)
+		if (flags & OTP_UPPER_HALF)
 			++page;
 		bytes_done += sizeof(content);
 		*pos += sizeof(content);
@@ -108,14 +78,53 @@ static ssize_t bfin_otp_read(struct file *file, char __user *buff, size_t count,
 }
 
 #ifdef CONFIG_BFIN_OTP_WRITE_ENABLE
+static bool allow_writes;
+
+/**
+ *	bfin_otp_init_timing - setup OTP timing parameters
+ *
+ *	Required before doing any write operation.  Algorithms from HRM.
+ */
+static u32 bfin_otp_init_timing(void)
+{
+	u32 tp1, tp2, tp3, timing;
+
+	tp1 = get_sclk() / 1000000;
+	tp2 = (2 * get_sclk() / 10000000) << 8;
+	tp3 = (0x1401) << 15;
+	timing = tp1 | tp2 | tp3;
+	if (bfrom_OtpCommand(OTP_INIT, timing))
+		return 0;
+
+	return timing;
+}
+
+/**
+ *	bfin_otp_deinit_timing - set timings to only allow reads
+ *
+ *	Should be called after all writes are done.
+ */
+static void bfin_otp_deinit_timing(u32 timing)
+{
+	/* mask bits [31:15] so that any attempts to write fail */
+	bfrom_OtpCommand(OTP_CLOSE, 0);
+	bfrom_OtpCommand(OTP_INIT, timing & ~(-1 << 15));
+	bfrom_OtpCommand(OTP_CLOSE, 0);
+}
+
 /**
- *	bfin_otp_write - Write OTP pages
+ *	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();
+	ssize_t bytes_done;
+	u32 timing, page, base_flags, flags, ret;
+	u64 content;
+
+	if (!allow_writes)
+		return -EACCES;
 
 	if (count % sizeof(u64))
 		return -EMSGSIZE;
@@ -123,20 +132,96 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
 	if (mutex_lock_interruptible(&bfin_otp_lock))
 		return -ERESTARTSYS;
 
-	/* need otp_init() documentation before this can be implemented */
+	stampit();
+
+	timing = bfin_otp_init_timing();
+	if (timing == 0) {
+		mutex_unlock(&bfin_otp_lock);
+		return -EIO;
+	}
+
+	base_flags = OTP_CHECK_FOR_PREV_WRITE;
+
+	bytes_done = 0;
+	page = *pos / (sizeof(u64) * 2);
+	while (bytes_done < count) {
+		flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
+		stamp("processing page %i (0x%x:%s) from %p", page, flags,
+			(flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
+		if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
+			bytes_done = -EFAULT;
+			break;
+		}
+		ret = bfrom_OtpWrite(page, flags, &content);
+		if (ret & OTP_MASTER_ERROR) {
+			stamp("error from otp: 0x%x", ret);
+			bytes_done = -EIO;
+			break;
+		}
+		if (flags & OTP_UPPER_HALF)
+			++page;
+		bytes_done += sizeof(content);
+		*pos += sizeof(content);
+	}
+
+	bfin_otp_deinit_timing(timing);
 
 	mutex_unlock(&bfin_otp_lock);
 
+	return bytes_done;
+}
+
+static long bfin_otp_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
+{
+	stampit();
+
+	switch (cmd) {
+	case OTPLOCK: {
+		u32 timing;
+		int ret = -EIO;
+
+		if (!allow_writes)
+			return -EACCES;
+
+		if (mutex_lock_interruptible(&bfin_otp_lock))
+			return -ERESTARTSYS;
+
+		timing = bfin_otp_init_timing();
+		if (timing) {
+			u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
+			stamp("locking page %lu resulted in 0x%x", arg, otp_result);
+			if (!(otp_result & OTP_MASTER_ERROR))
+				ret = 0;
+
+			bfin_otp_deinit_timing(timing);
+		}
+
+		mutex_unlock(&bfin_otp_lock);
+
+		return ret;
+	}
+
+	case MEMLOCK:
+		allow_writes = false;
+		return 0;
+
+	case MEMUNLOCK:
+		allow_writes = true;
+		return 0;
+	}
+
 	return -EINVAL;
 }
 #else
 # define bfin_otp_write NULL
+# define bfin_otp_ioctl NULL
 #endif
 
 static struct file_operations bfin_otp_fops = {
-	.owner    = THIS_MODULE,
-	.read     = bfin_otp_read,
-	.write    = bfin_otp_write,
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = bfin_otp_ioctl,
+	.read           = bfin_otp_read,
+	.write          = bfin_otp_write,
 };
 
 static struct miscdevice bfin_otp_misc_device = {
-- 
1.6.5.rc1