[RFC][PATCH] Add NAND lock/unlock routines

[RFC][PATCH] Add NAND lock/unlock routines

[Vimal Singh]

I am not sure how useful it will be, but still here is a patch for review.
-vimal

From: Vimal Singh <vimalsingh@ti.com>
Date: Tue, 24 Nov 2009 18:26:43 +0530
Subject: [PATCH] Add NAND lock/unlock routines

At least 'Micron' NAND parts have lock/unlock feature.
Adding routines for this.

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h     |    6 +
 2 files changed, 221 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 2957cc7..e447c24 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
struct nand_chip *chip)
 }

 /**
+ * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ * @invert -  when = 0, unlock the range of blocks within the lower and
+ *                      upper boundary address
+ *            whne = 1, unlock the range of blocks outside the boundaries
+ *                      of the lower and upper boundary address
+ *
+ * @return - unlock status
+ */
+static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
+					uint64_t len, int invert)
+{
+	int ret = 0;
+	int status, page;
+	struct nand_chip *chip = mtd->priv;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+			__func__, (unsigned long long)ofs, len);
+
+	/* Submit address of first page to unlock */
+	page = (int)(ofs >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
+
+	/* Submit address of last page to unlock */
+	page = (int)((ofs + len) >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
+				((page | invert) & chip->pagemask));
+
+	/* Call wait ready function */
+	status = chip->waitfunc(mtd, chip);
+	udelay(1000);
+	/* See if device thinks it succeeded */
+	if (status & 0x01) {
+		/* There was an error */
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+					__func__, status);
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+/**
+ * nand_unlock - [REPLACABLE] unlocks specified locked blockes
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ *
+ * @return - unlock status
+ */
+static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	int ret = 0;
+	int chipnr;
+	struct nand_chip *chip = mtd->priv;
+
+	/* Start address must align on block boundary */
+	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Length must align on block boundary */
+	if (len & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Do not allow past end of device */
+	if ((ofs + len) > mtd->size) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Align to last block address if size addresses end of the device */
+	if ((ofs + len) == mtd->size)
+		len -= mtd->erasesize;
+
+	/* Grab the lock and see if the device is available */
+	nand_get_device(chip, mtd, FL_UNLOCKING);
+
+	/* Shift to get chip number */
+	chipnr = (int)(ofs >> chip->chip_shift);
+
+	/* Select the NAND device */
+	chip->select_chip(mtd, chipnr);
+
+	/* Check, if it is write protected */
+	if (nand_check_wp(mtd)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+					__func__);
+		ret = -EIO;
+		goto out;
+	}
+
+	ret = __nand_unlock(mtd, ofs, len, 0);
+
+out:
+	/* de-select the NAND device */
+	chip->select_chip(mtd, -1);
+
+	/* Deselect and wake up anyone waiting on the device */
+	nand_release_device(mtd);
+
+	return ret;
+}
+
+/**
+ * nand_lock - [REPLACABLE] locks all blockes present in the device
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ *
+ * @return - lock status
+ *
+ * This feature is not support in many NAND parts. 'Micron' NAND parts
+ * do have this feature, but it allows only to lock all blocks not for
+ * specified range for block.
+ *
+ * Implementing 'lock' feature by making use of 'unlock', for now.
+ */
+static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	int ret = 0;
+	int chipnr, status, page;
+	struct nand_chip *chip = mtd->priv;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+			__func__, (unsigned long long)ofs, len);
+
+	/* Start address must align on block boundary */
+	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Length must align on block boundary */
+	if (len & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Do not allow past end of device */
+	if ((ofs + len) > mtd->size) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Grab the lock and see if the device is available */
+	nand_get_device(chip, mtd, FL_LOCKING);
+
+	/* Shift to get first page */
+	page = (int)(ofs >> chip->page_shift);
+	chipnr = (int)(ofs >> chip->chip_shift);
+
+	/* Select the NAND device */
+	chip->select_chip(mtd, chipnr);
+
+	/* Check, if it is write protected */
+	if (nand_check_wp(mtd)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+					__func__);
+		status = MTD_ERASE_FAILED;
+		ret = -EIO;
+		goto out;
+	}
+
+	/* Submit address of first page to unlock */
+	page = (int)(ofs >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_LOCK, -1, page & chip->pagemask);
+
+	/* Call wait ready function */
+	status = chip->waitfunc(mtd, chip);
+	udelay(1000);
+	/* See if device thinks it succeeded */
+	if (status & 0x01) {
+		/* There was an error */
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+					__func__, status);
+		ret = -EIO;
+		goto out;
+	}
+
+	if (len != -1)
+		ret = __nand_unlock(mtd, ofs, len, 0x1);
+
+out:
+	/* de-select the NAND device */
+	chip->select_chip(mtd, -1);
+
+	/* Deselect and wake up anyone waiting on the device */
+	nand_release_device(mtd);
+
+	return ret;
+}
+
+/**
  * nand_read_page_raw - [Intern] read raw page data without ecc
  * @mtd:	mtd info structure
  * @chip:	nand chip info structure
@@ -2257,6 +2469,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
erase_info *instr,

 		status = chip->waitfunc(mtd, chip);

+printk(KERN_INFO"VIMAL: status: 0x%x\n",status);
 		/*
 		 * See if operation failed and additional status checks are
 		 * available
@@ -2880,8 +3093,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->read_oob = nand_read_oob;
 	mtd->write_oob = nand_write_oob;
 	mtd->sync = nand_sync;
-	mtd->lock = NULL;
-	mtd->unlock = NULL;
+	mtd->lock = nand_lock;
+	mtd->unlock = nand_unlock;
 	mtd->suspend = nand_suspend;
 	mtd->resume = nand_resume;
 	mtd->block_isbad = nand_block_isbad;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7a232a9..f2d97ea 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -80,6 +80,10 @@ extern void nand_wait_ready(struct mtd_info *mtd);
 #define NAND_CMD_ERASE2		0xd0
 #define NAND_CMD_RESET		0xff

+#define NAND_CMD_LOCK		0x2a
+#define NAND_CMD_UNLOCK1	0x23
+#define NAND_CMD_UNLOCK2	0x24
+
 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART	0x30
 #define NAND_CMD_RNDOUTSTART	0xE0
@@ -214,6 +218,8 @@ typedef enum {
 	FL_SYNCING,
 	FL_CACHEDPRG,
 	FL_PM_SUSPENDED,
+	FL_LOCKING,
+	FL_UNLOCKING,
 } nand_state_t;

 /* Keep gcc happy */
-- 
1.5.5

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Artem Bityutskiy]

Hi, some cosmetic comments:

On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
> I am not sure how useful it will be, but still here is a patch for review.
> -vimal
> 
> From: Vimal Singh <vimalsingh@ti.com>
> Date: Tue, 24 Nov 2009 18:26:43 +0530
> Subject: [PATCH] Add NAND lock/unlock routines
> 
> At least 'Micron' NAND parts have lock/unlock feature.
> Adding routines for this.
> 
> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> ---
>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/nand.h     |    6 +
>  2 files changed, 221 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 2957cc7..e447c24 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
> struct nand_chip *chip)
>  }
> 
>  /**
> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> + *
> + * @param mtd - mtd info
> + * @param ofs - offset to start unlock from
> + * @param len - length to unlock
> + * @invert -  when = 0, unlock the range of blocks within the lower and
> + *                      upper boundary address
> + *            whne = 1, unlock the range of blocks outside the boundaries
> + *                      of the lower and upper boundary address
> + *
> + * @return - unlock status
> + */
> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> +					uint64_t len, int invert)
> +{
> +	int ret = 0;
> +	int status, page;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> +			__func__, (unsigned long long)ofs, len);
> +
> +	/* Submit address of first page to unlock */
> +	page = (int)(ofs >> chip->page_shift);

The compiler will automatically cast the result to int I believe.

> +	chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
> +
> +	/* Submit address of last page to unlock */
> +	page = (int)((ofs + len) >> chip->page_shift);
Ditto.

> +	chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
> +				((page | invert) & chip->pagemask));
> +
> +	/* Call wait ready function */
> +	status = chip->waitfunc(mtd, chip);
> +	udelay(1000);
> +	/* See if device thinks it succeeded */
> +	if (status & 0x01) {
> +		/* There was an error */
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
> +					__func__, status);
> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * nand_unlock - [REPLACABLE] unlocks specified locked blockes
> + *
> + * @param mtd - mtd info
> + * @param ofs - offset to start unlock from
> + * @param len - length to unlock
> + *
> + * @return - unlock status
> + */
> +static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +	int ret = 0;
> +	int chipnr;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	/* Start address must align on block boundary */
> +	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Length must align on block boundary */
> +	if (len & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Do not allow past end of device */
> +	if ((ofs + len) > mtd->size) {

() around ofs + len look a bit silly for me

> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Align to last block address if size addresses end of the device */
> +	if ((ofs + len) == mtd->size)
> +		len -= mtd->erasesize;

And here
> +
> +	/* Grab the lock and see if the device is available */
> +	nand_get_device(chip, mtd, FL_UNLOCKING);
> +
> +	/* Shift to get chip number */
> +	chipnr = (int)(ofs >> chip->chip_shift);

I do not think explicit cast is needed.

> +
> +	/* Select the NAND device */
> +	chip->select_chip(mtd, chipnr);
> +
> +	/* Check, if it is write protected */
> +	if (nand_check_wp(mtd)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
> +					__func__);
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	ret = __nand_unlock(mtd, ofs, len, 0);
> +
> +out:
> +	/* de-select the NAND device */
> +	chip->select_chip(mtd, -1);
> +
> +	/* Deselect and wake up anyone waiting on the device */
> +	nand_release_device(mtd);
> +
> +	return ret;
> +}

...

And take a look at the same things in the rest of the code.

> +/**
>   * nand_read_page_raw - [Intern] read raw page data without ecc
>   * @mtd:	mtd info structure
>   * @chip:	nand chip info structure
> @@ -2257,6 +2469,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
> erase_info *instr,
> 
>  		status = chip->waitfunc(mtd, chip);
> 
> +printk(KERN_INFO"VIMAL: status: 0x%x\n",status);

Forgot to remove a debugging printk?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Vimal Singh]

On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Hi, some cosmetic comments:
>
> On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
>> I am not sure how useful it will be, but still here is a patch for review.
>> -vimal
>>
>> From: Vimal Singh <vimalsingh@ti.com>
>> Date: Tue, 24 Nov 2009 18:26:43 +0530
>> Subject: [PATCH] Add NAND lock/unlock routines
>>
>> At least 'Micron' NAND parts have lock/unlock feature.
>> Adding routines for this.
>>
>> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mtd/nand.h     |    6 +
>>  2 files changed, 221 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 2957cc7..e447c24 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
>> struct nand_chip *chip)
>>  }
>>
>>  /**
>> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> + *
>> + * @param mtd - mtd info
>> + * @param ofs - offset to start unlock from
>> + * @param len - length to unlock
>> + * @invert -  when = 0, unlock the range of blocks within the lower and
>> + *                      upper boundary address
>> + *            whne = 1, unlock the range of blocks outside the boundaries
>> + *                      of the lower and upper boundary address
>> + *
>> + * @return - unlock status
>> + */
>> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
>> +                                     uint64_t len, int invert)
>> +{
>> +     int ret = 0;
>> +     int status, page;
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> +                     __func__, (unsigned long long)ofs, len);
>> +
>> +     /* Submit address of first page to unlock */
>> +     page = (int)(ofs >> chip->page_shift);
>
> The compiler will automatically cast the result to int I believe.

I just copied this line from erase functions.
I believe its better to cast here as otherwise we may see compiler warnings.

>
>> +     chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
>> +
>> +     /* Submit address of last page to unlock */
>> +     page = (int)((ofs + len) >> chip->page_shift);
> Ditto.
>
>> +     chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
>> +                             ((page | invert) & chip->pagemask));
>> +
>> +     /* Call wait ready function */
>> +     status = chip->waitfunc(mtd, chip);
>> +     udelay(1000);
>> +     /* See if device thinks it succeeded */
>> +     if (status & 0x01) {
>> +             /* There was an error */
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
>> +                                     __func__, status);
>> +             ret = -EIO;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>> + * nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> + *
>> + * @param mtd - mtd info
>> + * @param ofs - offset to start unlock from
>> + * @param len - length to unlock
>> + *
>> + * @return - unlock status
>> + */
>> +static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +{
>> +     int ret = 0;
>> +     int chipnr;
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     /* Start address must align on block boundary */
>> +     if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Length must align on block boundary */
>> +     if (len & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Do not allow past end of device */
>> +     if ((ofs + len) > mtd->size) {
>
> () around ofs + len look a bit silly for me

This is again used from old code. But I can remove it.

>
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Align to last block address if size addresses end of the device */
>> +     if ((ofs + len) == mtd->size)
>> +             len -= mtd->erasesize;
>
> And here
ok

>> +
>> +     /* Grab the lock and see if the device is available */
>> +     nand_get_device(chip, mtd, FL_UNLOCKING);
>> +
>> +     /* Shift to get chip number */
>> +     chipnr = (int)(ofs >> chip->chip_shift);
>
> I do not think explicit cast is needed.

same comment as previous one
>
>> +
>> +     /* Select the NAND device */
>> +     chip->select_chip(mtd, chipnr);
>> +
>> +     /* Check, if it is write protected */
>> +     if (nand_check_wp(mtd)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
>> +                                     __func__);
>> +             ret = -EIO;
>> +             goto out;
>> +     }
>> +
>> +     ret = __nand_unlock(mtd, ofs, len, 0);
>> +
>> +out:
>> +     /* de-select the NAND device */
>> +     chip->select_chip(mtd, -1);
>> +
>> +     /* Deselect and wake up anyone waiting on the device */
>> +     nand_release_device(mtd);
>> +
>> +     return ret;
>> +}
>
> ...
>
> And take a look at the same things in the rest of the code.
>
>> +/**
>>   * nand_read_page_raw - [Intern] read raw page data without ecc
>>   * @mtd:     mtd info structure
>>   * @chip:    nand chip info structure
>> @@ -2257,6 +2469,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
>> erase_info *instr,
>>
>>               status = chip->waitfunc(mtd, chip);
>>
>> +printk(KERN_INFO"VIMAL: status: 0x%x\n",status);
>
> Forgot to remove a debugging printk?

Yes, this is my mistake. I'll remove this and drop new version of this
patch again.

-- 
Regards,
Vimal Singh

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Artem Bityutskiy]

On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Hi, some cosmetic comments:
> >
> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
> >> I am not sure how useful it will be, but still here is a patch for review.
> >> -vimal
> >>
> >> From: Vimal Singh <vimalsingh@ti.com>
> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
> >> Subject: [PATCH] Add NAND lock/unlock routines
> >>
> >> At least 'Micron' NAND parts have lock/unlock feature.
> >> Adding routines for this.
> >>
> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> ---
> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
> >>  include/linux/mtd/nand.h     |    6 +
> >>  2 files changed, 221 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index 2957cc7..e447c24 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
> >> struct nand_chip *chip)
> >>  }
> >>
> >>  /**
> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> >> + *
> >> + * @param mtd - mtd info
> >> + * @param ofs - offset to start unlock from
> >> + * @param len - length to unlock
> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
> >> + *                      upper boundary address
> >> + *            whne = 1, unlock the range of blocks outside the boundaries
> >> + *                      of the lower and upper boundary address
> >> + *
> >> + * @return - unlock status
> >> + */
> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> >> +                                     uint64_t len, int invert)
> >> +{
> >> +     int ret = 0;
> >> +     int status, page;
> >> +     struct nand_chip *chip = mtd->priv;
> >> +
> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> >> +                     __func__, (unsigned long long)ofs, len);
> >> +
> >> +     /* Submit address of first page to unlock */
> >> +     page = (int)(ofs >> chip->page_shift);
> >
> > The compiler will automatically cast the result to int I believe.
> 
> I just copied this line from erase functions.
> I believe its better to cast here as otherwise we may see compiler warnings.

Good point. Could you please create a validation checking helper instead
of duplicating code?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Vimal Singh]

On Mon, Dec 7, 2009 at 2:59 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
>> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > Hi, some cosmetic comments:
>> >
>> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
>> >> I am not sure how useful it will be, but still here is a patch for review.
>> >> -vimal
>> >>
>> >> From: Vimal Singh <vimalsingh@ti.com>
>> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
>> >> Subject: [PATCH] Add NAND lock/unlock routines
>> >>
>> >> At least 'Micron' NAND parts have lock/unlock feature.
>> >> Adding routines for this.
>> >>
>> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> >> ---
>> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
>> >>  include/linux/mtd/nand.h     |    6 +
>> >>  2 files changed, 221 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> >> index 2957cc7..e447c24 100644
>> >> --- a/drivers/mtd/nand/nand_base.c
>> >> +++ b/drivers/mtd/nand/nand_base.c
>> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
>> >> struct nand_chip *chip)
>> >>  }
>> >>
>> >>  /**
>> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> >> + *
>> >> + * @param mtd - mtd info
>> >> + * @param ofs - offset to start unlock from
>> >> + * @param len - length to unlock
>> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
>> >> + *                      upper boundary address
>> >> + *            whne = 1, unlock the range of blocks outside the boundaries
>> >> + *                      of the lower and upper boundary address
>> >> + *
>> >> + * @return - unlock status
>> >> + */
>> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
>> >> +                                     uint64_t len, int invert)
>> >> +{
>> >> +     int ret = 0;
>> >> +     int status, page;
>> >> +     struct nand_chip *chip = mtd->priv;
>> >> +
>> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> >> +                     __func__, (unsigned long long)ofs, len);
>> >> +
>> >> +     /* Submit address of first page to unlock */
>> >> +     page = (int)(ofs >> chip->page_shift);
>> >
>> > The compiler will automatically cast the result to int I believe.
>>
>> I just copied this line from erase functions.
>> I believe its better to cast here as otherwise we may see compiler warnings.
>
> Good point. Could you please create a validation checking helper instead
> of duplicating code?

IMHO that should be done in a separate patch.


-- 
Regards,
Vimal Singh

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Artem Bityutskiy]

On Mon, 2009-12-07 at 16:36 +0530, Vimal Singh wrote:
> On Mon, Dec 7, 2009 at 2:59 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
> >> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > Hi, some cosmetic comments:
> >> >
> >> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
> >> >> I am not sure how useful it will be, but still here is a patch for review.
> >> >> -vimal
> >> >>
> >> >> From: Vimal Singh <vimalsingh@ti.com>
> >> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
> >> >> Subject: [PATCH] Add NAND lock/unlock routines
> >> >>
> >> >> At least 'Micron' NAND parts have lock/unlock feature.
> >> >> Adding routines for this.
> >> >>
> >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> >> ---
> >> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
> >> >>  include/linux/mtd/nand.h     |    6 +
> >> >>  2 files changed, 221 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> >> index 2957cc7..e447c24 100644
> >> >> --- a/drivers/mtd/nand/nand_base.c
> >> >> +++ b/drivers/mtd/nand/nand_base.c
> >> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
> >> >> struct nand_chip *chip)
> >> >>  }
> >> >>
> >> >>  /**
> >> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> >> >> + *
> >> >> + * @param mtd - mtd info
> >> >> + * @param ofs - offset to start unlock from
> >> >> + * @param len - length to unlock
> >> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
> >> >> + *                      upper boundary address
> >> >> + *            whne = 1, unlock the range of blocks outside the boundaries
> >> >> + *                      of the lower and upper boundary address
> >> >> + *
> >> >> + * @return - unlock status
> >> >> + */
> >> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> >> >> +                                     uint64_t len, int invert)
> >> >> +{
> >> >> +     int ret = 0;
> >> >> +     int status, page;
> >> >> +     struct nand_chip *chip = mtd->priv;
> >> >> +
> >> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> >> >> +                     __func__, (unsigned long long)ofs, len);
> >> >> +
> >> >> +     /* Submit address of first page to unlock */
> >> >> +     page = (int)(ofs >> chip->page_shift);
> >> >
> >> > The compiler will automatically cast the result to int I believe.
> >>
> >> I just copied this line from erase functions.
> >> I believe its better to cast here as otherwise we may see compiler warnings.
> >
> > Good point. Could you please create a validation checking helper instead
> > of duplicating code?
> 
> IMHO that should be done in a separate patch.

Right, you can first send this as a separate patch, and then the rest as
a follow up one.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Vimal Singh]

On Mon, Dec 7, 2009 at 4:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2009-12-07 at 16:36 +0530, Vimal Singh wrote:
>> On Mon, Dec 7, 2009 at 2:59 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
>> >> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> >> > Hi, some cosmetic comments:
>> >> >
>> >> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
>> >> >> I am not sure how useful it will be, but still here is a patch for review.
>> >> >> -vimal
>> >> >>
>> >> >> From: Vimal Singh <vimalsingh@ti.com>
>> >> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
>> >> >> Subject: [PATCH] Add NAND lock/unlock routines
>> >> >>
>> >> >> At least 'Micron' NAND parts have lock/unlock feature.
>> >> >> Adding routines for this.
>> >> >>
>> >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> >> >> ---
>> >> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
>> >> >>  include/linux/mtd/nand.h     |    6 +
>> >> >>  2 files changed, 221 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> >> >> index 2957cc7..e447c24 100644
>> >> >> --- a/drivers/mtd/nand/nand_base.c
>> >> >> +++ b/drivers/mtd/nand/nand_base.c
>> >> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
>> >> >> struct nand_chip *chip)
>> >> >>  }
>> >> >>
>> >> >>  /**
>> >> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> >> >> + *
>> >> >> + * @param mtd - mtd info
>> >> >> + * @param ofs - offset to start unlock from
>> >> >> + * @param len - length to unlock
>> >> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
>> >> >> + *                      upper boundary address
>> >> >> + *            whne = 1, unlock the range of blocks outside the boundaries
>> >> >> + *                      of the lower and upper boundary address
>> >> >> + *
>> >> >> + * @return - unlock status
>> >> >> + */
>> >> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
>> >> >> +                                     uint64_t len, int invert)
>> >> >> +{
>> >> >> +     int ret = 0;
>> >> >> +     int status, page;
>> >> >> +     struct nand_chip *chip = mtd->priv;
>> >> >> +
>> >> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> >> >> +                     __func__, (unsigned long long)ofs, len);
>> >> >> +
>> >> >> +     /* Submit address of first page to unlock */
>> >> >> +     page = (int)(ofs >> chip->page_shift);
>> >> >
>> >> > The compiler will automatically cast the result to int I believe.
>> >>
>> >> I just copied this line from erase functions.
>> >> I believe its better to cast here as otherwise we may see compiler warnings.
>> >
>> > Good point. Could you please create a validation checking helper instead
>> > of duplicating code?
>>
>> IMHO that should be done in a separate patch.
>
> Right, you can first send this as a separate patch, and then the rest as
> a follow up one.

when I went back on validation checking's I notice:

-nand_read: does validation for access to past end of the device
-nand_do_read_oob: does it for access to past oob and device.
-nand_read_oob: does for access to past end of the device

-nand_write: does it for access to past end of the device
-nand_do_write_ops: does it for page alignment
-nand_do_write_oob: does it for access to past oob and device and page
alignment
-panic_nand_write: does it for access to past end of the device

-nand_erase_nand: does it for access to past end of the device and
block alignment 'lock/unlock' routines are doing same validations as
'nand_erase_nand' does.

There is no consistancy in validation checks other than between
'erase' and 'lock/unlock'.
Now since currently only 'erase' function does those validations. We
can have patch for separate validation functions only after
'lock/unlock' patch.

Any comment or suggestions?

-- 
Regards,
Vimal Singh

[PATCH - V2] Add NAND lock/unlock routines

[Vimal Singh]

Here is the 2nd version of this patch after removing extra braces as
suggested by Artem.

-vimal

>From 2dc40c3d42c14ac7e73ec8a9a1ccf9e524385263 Mon Sep 17 00:00:00 2001
From: Vimal Singh <vimalsingh@ti.com>
Date: Wed, 6 Jan 2010 18:13:01 +0530
Subject: [PATCH] Add NAND lock/unlock routines

At least 'Micron' NAND parts have lock/unlock feature.
Adding routines for this.

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 drivers/mtd/nand/nand_base.c |  216 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h     |    4 +
 2 files changed, 218 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8f2958f..2ceec1c 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -835,6 +835,218 @@ static int nand_wait(struct mtd_info
 }

 /**
+ * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ * @invert -  when = 0, unlock the range of blocks within the lower and
+ *                      upper boundary address
+ *            whne = 1, unlock the range of blocks outside the boundaries
+ *                      of the lower and upper boundary address
+ *
+ * @return - unlock status
+ */
+static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
+					uint64_t len, int invert)
+{
+	int ret = 0;
+	int status, page;
+	struct nand_chip *chip = mtd->priv;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+			__func__, (unsigned long long)ofs, len);
+
+	/* Submit address of first page to unlock */
+	page = (int)(ofs >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
+
+	/* Submit address of last page to unlock */
+	page = (int)(ofs + len >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
+				((page | invert) & chip->pagemask));
+
+	/* Call wait ready function */
+	status = chip->waitfunc(mtd, chip);
+	udelay(1000);
+	/* See if device thinks it succeeded */
+	if (status & 0x01) {
+		/* There was an error */
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+					__func__, status);
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+/**
+ * nand_unlock - [REPLACABLE] unlocks specified locked blockes
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ *
+ * @return - unlock status
+ */
+static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	int ret = 0;
+	int chipnr;
+	struct nand_chip *chip = mtd->priv;
+
+	/* Start address must align on block boundary */
+	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Length must align on block boundary */
+	if (len & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Do not allow past end of device */
+	if (ofs + len > mtd->size) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Align to last block address if size addresses end of the device */
+	if (ofs + len == mtd->size)
+		len -= mtd->erasesize;
+
+	/* Grab the lock and see if the device is available */
+	nand_get_device(chip, mtd, FL_UNLOCKING);
+
+	/* Shift to get chip number */
+	chipnr = (int)(ofs >> chip->chip_shift);
+
+	/* Select the NAND device */
+	chip->select_chip(mtd, chipnr);
+
+	/* Check, if it is write protected */
+	if (nand_check_wp(mtd)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+					__func__);
+		ret = -EIO;
+		goto out;
+	}
+
+	ret = __nand_unlock(mtd, ofs, len, 0);
+
+out:
+	/* de-select the NAND device */
+	chip->select_chip(mtd, -1);
+
+	/* Deselect and wake up anyone waiting on the device */
+	nand_release_device(mtd);
+
+	return ret;
+}
+
+/**
+ * nand_lock - [REPLACABLE] locks all blockes present in the device
+ *
+ * @param mtd - mtd info
+ * @param ofs - offset to start unlock from
+ * @param len - length to unlock
+ *
+ * @return - lock status
+ *
+ * This feature is not support in many NAND parts. 'Micron' NAND parts
+ * do have this feature, but it allows only to lock all blocks not for
+ * specified range for block.
+ *
+ * Implementing 'lock' feature by making use of 'unlock', for now.
+ */
+static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	int ret = 0;
+	int chipnr, status, page;
+	struct nand_chip *chip = mtd->priv;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+			__func__, (unsigned long long)ofs, len);
+
+	/* Start address must align on block boundary */
+	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Length must align on block boundary */
+	if (len & ((1 << chip->phys_erase_shift) - 1)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Do not allow past end of device */
+	if (ofs + len > mtd->size) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
+					__func__);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Grab the lock and see if the device is available */
+	nand_get_device(chip, mtd, FL_LOCKING);
+
+	/* Shift to get first page */
+	page = (int)(ofs >> chip->page_shift);
+	chipnr = (int)(ofs >> chip->chip_shift);
+
+	/* Select the NAND device */
+	chip->select_chip(mtd, chipnr);
+
+	/* Check, if it is write protected */
+	if (nand_check_wp(mtd)) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+					__func__);
+		status = MTD_ERASE_FAILED;
+		ret = -EIO;
+		goto out;
+	}
+
+	/* Submit address of first page to unlock */
+	page = (int)(ofs >> chip->page_shift);
+	chip->cmdfunc(mtd, NAND_CMD_LOCK, -1, page & chip->pagemask);
+
+	/* Call wait ready function */
+	status = chip->waitfunc(mtd, chip);
+	udelay(1000);
+	/* See if device thinks it succeeded */
+	if (status & 0x01) {
+		/* There was an error */
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+					__func__, status);
+		ret = -EIO;
+		goto out;
+	}
+
+	if (len != -1)
+		ret = __nand_unlock(mtd, ofs, len, 0x1);
+
+out:
+	/* de-select the NAND device */
+	chip->select_chip(mtd, -1);
+
+	/* Deselect and wake up anyone waiting on the device */
+	nand_release_device(mtd);
+
+	return ret;
+}
+
+/**
  * nand_read_page_raw - [Intern] read raw page data without ecc
  * @mtd:	mtd info structure
  * @chip:	nand chip info structure
@@ -2999,8 +3211,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->read_oob = nand_read_oob;
 	mtd->write_oob = nand_write_oob;
 	mtd->sync = nand_sync;
-	mtd->lock = NULL;
-	mtd->unlock = NULL;
+	mtd->lock = nand_lock;
+	mtd->unlock = nand_unlock;
 	mtd->suspend = nand_suspend;
 	mtd->resume = nand_resume;
 	mtd->block_isbad = nand_block_isbad;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index ccab9df..698eada 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -82,6 +82,10 @@ extern void nand_wait_ready(struct mtd_info *mtd);
 #define NAND_CMD_ERASE2		0xd0
 #define NAND_CMD_RESET		0xff

+#define NAND_CMD_LOCK		0x2a
+#define NAND_CMD_UNLOCK1	0x23
+#define NAND_CMD_UNLOCK2	0x24
+
 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART	0x30
 #define NAND_CMD_RNDOUTSTART	0xE0
-- 
1.5.5

Re: [RFC][PATCH] Add NAND lock/unlock routines

[Artem Bityutskiy]

On Thu, 2009-12-17 at 15:11 +0530, Vimal Singh wrote:
> On Mon, Dec 7, 2009 at 4:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Mon, 2009-12-07 at 16:36 +0530, Vimal Singh wrote:
> >> On Mon, Dec 7, 2009 at 2:59 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
> >> >> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> >> > Hi, some cosmetic comments:
> >> >> >
> >> >> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
> >> >> >> I am not sure how useful it will be, but still here is a patch for review.
> >> >> >> -vimal
> >> >> >>
> >> >> >> From: Vimal Singh <vimalsingh@ti.com>
> >> >> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
> >> >> >> Subject: [PATCH] Add NAND lock/unlock routines
> >> >> >>
> >> >> >> At least 'Micron' NAND parts have lock/unlock feature.
> >> >> >> Adding routines for this.
> >> >> >>
> >> >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> >> >> ---
> >> >> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
> >> >> >>  include/linux/mtd/nand.h     |    6 +
> >> >> >>  2 files changed, 221 insertions(+), 2 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> >> >> index 2957cc7..e447c24 100644
> >> >> >> --- a/drivers/mtd/nand/nand_base.c
> >> >> >> +++ b/drivers/mtd/nand/nand_base.c
> >> >> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
> >> >> >> struct nand_chip *chip)
> >> >> >>  }
> >> >> >>
> >> >> >>  /**
> >> >> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> >> >> >> + *
> >> >> >> + * @param mtd - mtd info
> >> >> >> + * @param ofs - offset to start unlock from
> >> >> >> + * @param len - length to unlock
> >> >> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
> >> >> >> + *                      upper boundary address
> >> >> >> + *            whne = 1, unlock the range of blocks outside the boundaries
> >> >> >> + *                      of the lower and upper boundary address
> >> >> >> + *
> >> >> >> + * @return - unlock status
> >> >> >> + */
> >> >> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> >> >> >> +                                     uint64_t len, int invert)
> >> >> >> +{
> >> >> >> +     int ret = 0;
> >> >> >> +     int status, page;
> >> >> >> +     struct nand_chip *chip = mtd->priv;
> >> >> >> +
> >> >> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> >> >> >> +                     __func__, (unsigned long long)ofs, len);
> >> >> >> +
> >> >> >> +     /* Submit address of first page to unlock */
> >> >> >> +     page = (int)(ofs >> chip->page_shift);
> >> >> >
> >> >> > The compiler will automatically cast the result to int I believe.
> >> >>
> >> >> I just copied this line from erase functions.
> >> >> I believe its better to cast here as otherwise we may see compiler warnings.
> >> >
> >> > Good point. Could you please create a validation checking helper instead
> >> > of duplicating code?
> >>
> >> IMHO that should be done in a separate patch.
> >
> > Right, you can first send this as a separate patch, and then the rest as
> > a follow up one.
> 
> when I went back on validation checking's I notice:
> 
> -nand_read: does validation for access to past end of the device
> -nand_do_read_oob: does it for access to past oob and device.
> -nand_read_oob: does for access to past end of the device
> 
> -nand_write: does it for access to past end of the device
> -nand_do_write_ops: does it for page alignment
> -nand_do_write_oob: does it for access to past oob and device and page
> alignment
> -panic_nand_write: does it for access to past end of the device
> 
> -nand_erase_nand: does it for access to past end of the device and
> block alignment 'lock/unlock' routines are doing same validations as
> 'nand_erase_nand' does.
> 
> There is no consistancy in validation checks other than between
> 'erase' and 'lock/unlock'.
> Now since currently only 'erase' function does those validations. We
> can have patch for separate validation functions only after
> 'lock/unlock' patch.
> 
> Any comment or suggestions?

Well, of course my suggestion is to make that as consistent as possible,
and send a series of patches.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH - V2] Add NAND lock/unlock routines

[Artem Bityutskiy]

Hi Vimal,

please, find my nit-picks below :-)

On Wed, 2010-01-06 at 19:18 +0530, Vimal Singh wrote:
> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> ---
>  drivers/mtd/nand/nand_base.c |  216 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/nand.h     |    4 +
>  2 files changed, 218 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 8f2958f..2ceec1c 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -835,6 +835,218 @@ static int nand_wait(struct mtd_info
>  }
> 
>  /**
> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> + *
> + * @param mtd - mtd info
> + * @param ofs - offset to start unlock from
> + * @param len - length to unlock
> + * @invert -  when = 0, unlock the range of blocks within the lower and
> + *                      upper boundary address
> + *            whne = 1, unlock the range of blocks outside the boundaries
> + *                      of the lower and upper boundary address
> + *
> + * @return - unlock status
> + */
> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> +					uint64_t len, int invert)
> +{
> +	int ret = 0;
> +	int status, page;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> +			__func__, (unsigned long long)ofs, len);
> +
> +	/* Submit address of first page to unlock */
> +	page = (int)(ofs >> chip->page_shift);

Why this cast is needed?

> +	chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
> +
> +	/* Submit address of last page to unlock */
> +	page = (int)(ofs + len >> chip->page_shift);

And this?

> +	chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
> +				((page | invert) & chip->pagemask));

Why the third operand requires additional braces?

> +
> +	/* Call wait ready function */
> +	status = chip->waitfunc(mtd, chip);
> +	udelay(1000);
> +	/* See if device thinks it succeeded */
> +	if (status & 0x01) {
> +		/* There was an error */
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
> +					__func__, status);
> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * nand_unlock - [REPLACABLE] unlocks specified locked blockes
> + *
> + * @param mtd - mtd info
> + * @param ofs - offset to start unlock from
> + * @param len - length to unlock
> + *
> + * @return - unlock status
> + */
> +static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +	int ret = 0;
> +	int chipnr;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	/* Start address must align on block boundary */
> +	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Length must align on block boundary */
> +	if (len & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Do not allow past end of device */
> +	if (ofs + len > mtd->size) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}

To make this nicely, you need to create a helper for checking. And then
try to use that helper for other functions as well.

> +
> +	/* Align to last block address if size addresses end of the device */
> +	if (ofs + len == mtd->size)
> +		len -= mtd->erasesize;
> +
> +	/* Grab the lock and see if the device is available */
> +	nand_get_device(chip, mtd, FL_UNLOCKING);

I understand that you copied some pieces of code. But I think the
comment about is very redundant. It repeats everywhere, and comments an
obvious thing.

> +
> +	/* Shift to get chip number */
> +	chipnr = (int)(ofs >> chip->chip_shift);
> +
> +	/* Select the NAND device */
> +	chip->select_chip(mtd, chipnr);

This is a similar on, IMHO.

> +
> +	/* Check, if it is write protected */
> +	if (nand_check_wp(mtd)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
> +					__func__);
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	ret = __nand_unlock(mtd, ofs, len, 0);
> +
> +out:
> +	/* de-select the NAND device */
> +	chip->select_chip(mtd, -1);
> +
> +	/* Deselect and wake up anyone waiting on the device */
> +	nand_release_device(mtd);
And the above 2.

And similar useless comments are in the 'lock' function..

> +
> +	return ret;
> +}
> +
> +/**
> + * nand_lock - [REPLACABLE] locks all blockes present in the device
> + *
> + * @param mtd - mtd info
> + * @param ofs - offset to start unlock from
> + * @param len - length to unlock
> + *
> + * @return - lock status
> + *
> + * This feature is not support in many NAND parts. 'Micron' NAND parts
> + * do have this feature, but it allows only to lock all blocks not for
> + * specified range for block.
> + *
> + * Implementing 'lock' feature by making use of 'unlock', for now.
> + */
> +static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +	int ret = 0;
> +	int chipnr, status, page;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> +			__func__, (unsigned long long)ofs, len);
> +
> +	/* Start address must align on block boundary */
> +	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Length must align on block boundary */
> +	if (len & ((1 << chip->phys_erase_shift) - 1)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Do not allow past end of device */
> +	if (ofs + len > mtd->size) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
> +					__func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}

Repeated pattern, needs a helper function.

> +
> +	/* Grab the lock and see if the device is available */
> +	nand_get_device(chip, mtd, FL_LOCKING);
> +
> +	/* Shift to get first page */
> +	page = (int)(ofs >> chip->page_shift);
> +	chipnr = (int)(ofs >> chip->chip_shift);
> +
> +	/* Select the NAND device */
> +	chip->select_chip(mtd, chipnr);
> +
> +	/* Check, if it is write protected */
> +	if (nand_check_wp(mtd)) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
> +					__func__);
> +		status = MTD_ERASE_FAILED;
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	/* Submit address of first page to unlock */
> +	page = (int)(ofs >> chip->page_shift);
> +	chip->cmdfunc(mtd, NAND_CMD_LOCK, -1, page & chip->pagemask);
> +
> +	/* Call wait ready function */
> +	status = chip->waitfunc(mtd, chip);
> +	udelay(1000);
> +	/* See if device thinks it succeeded */
> +	if (status & 0x01) {
> +		/* There was an error */
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
> +					__func__, status);
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	if (len != -1)
> +		ret = __nand_unlock(mtd, ofs, len, 0x1);
> +
> +out:
> +	/* de-select the NAND device */
> +	chip->select_chip(mtd, -1);
> +
> +	/* Deselect and wake up anyone waiting on the device */
> +	nand_release_device(mtd);
> +
> +	return ret;
> +}
> +
> +/**
>   * nand_read_page_raw - [Intern] read raw page data without ecc
>   * @mtd:	mtd info structure
>   * @chip:	nand chip info structure
> @@ -2999,8 +3211,8 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	mtd->read_oob = nand_read_oob;
>  	mtd->write_oob = nand_write_oob;
>  	mtd->sync = nand_sync;
> -	mtd->lock = NULL;
> -	mtd->unlock = NULL;
> +	mtd->lock = nand_lock;
> +	mtd->unlock = nand_unlock;

What makes you believe it is safe to assign these call-backs here?

AFAICS, this means it will be done for all flashes. Do all of them
support lock/unlock? I did not investigate this, but I think that
probably not.


>  	mtd->suspend = nand_suspend;
>  	mtd->resume = nand_resume;
>  	mtd->block_isbad = nand_block_isbad;
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index ccab9df..698eada 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -82,6 +82,10 @@ extern void nand_wait_ready(struct mtd_info *mtd);
>  #define NAND_CMD_ERASE2		0xd0
>  #define NAND_CMD_RESET		0xff
> 
> +#define NAND_CMD_LOCK		0x2a
> +#define NAND_CMD_UNLOCK1	0x23
> +#define NAND_CMD_UNLOCK2	0x24
> +
>  /* Extended commands for large page devices */
>  #define NAND_CMD_READSTART	0x30
>  #define NAND_CMD_RNDOUTSTART	0xE0
-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH - V2] Add NAND lock/unlock routines

[Vimal Singh]

On Wed, Jan 13, 2010 at 2:10 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Hi Vimal,
>
> please, find my nit-picks below :-)
>
> On Wed, 2010-01-06 at 19:18 +0530, Vimal Singh wrote:
>> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |  216 +++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mtd/nand.h     |    4 +
>>  2 files changed, 218 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 8f2958f..2ceec1c 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -835,6 +835,218 @@ static int nand_wait(struct mtd_info
>>  }
>>
>>  /**
>> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> + *
>> + * @param mtd - mtd info
>> + * @param ofs - offset to start unlock from
>> + * @param len - length to unlock
>> + * @invert -  when = 0, unlock the range of blocks within the lower and
>> + *                      upper boundary address
>> + *            whne = 1, unlock the range of blocks outside the boundaries
>> + *                      of the lower and upper boundary address
>> + *
>> + * @return - unlock status
>> + */
>> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
>> +                                     uint64_t len, int invert)
>> +{
>> +     int ret = 0;
>> +     int status, page;
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> +                     __func__, (unsigned long long)ofs, len);
>> +
>> +     /* Submit address of first page to unlock */
>> +     page = (int)(ofs >> chip->page_shift);
>
> Why this cast is needed?

I'll remove this.

>
>> +     chip->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & chip->pagemask);
>> +
>> +     /* Submit address of last page to unlock */
>> +     page = (int)(ofs + len >> chip->page_shift);
>
> And this?

this too.

>
>> +     chip->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1,
>> +                             ((page | invert) & chip->pagemask));
>
> Why the third operand requires additional braces?

i'll correct it.

>
>> +
>> +     /* Call wait ready function */
>> +     status = chip->waitfunc(mtd, chip);
>> +     udelay(1000);
>> +     /* See if device thinks it succeeded */
>> +     if (status & 0x01) {
>> +             /* There was an error */
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
>> +                                     __func__, status);
>> +             ret = -EIO;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>> + * nand_unlock - [REPLACABLE] unlocks specified locked blockes
>> + *
>> + * @param mtd - mtd info
>> + * @param ofs - offset to start unlock from
>> + * @param len - length to unlock
>> + *
>> + * @return - unlock status
>> + */
>> +static int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +{
>> +     int ret = 0;
>> +     int chipnr;
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     /* Start address must align on block boundary */
>> +     if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Length must align on block boundary */
>> +     if (len & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Do not allow past end of device */
>> +     if (ofs + len > mtd->size) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>
> To make this nicely, you need to create a helper for checking. And then
> try to use that helper for other functions as well.

Next time I'll have a patch set where 1st patch will be this one and
2nd will do helper function work.

>
>> +
>> +     /* Align to last block address if size addresses end of the device */
>> +     if (ofs + len == mtd->size)
>> +             len -= mtd->erasesize;
>> +
>> +     /* Grab the lock and see if the device is available */
>> +     nand_get_device(chip, mtd, FL_UNLOCKING);
>
> I understand that you copied some pieces of code. But I think the
> comment about is very redundant. It repeats everywhere, and comments an
> obvious thing.

OK, Removing these in next version of the patch.

>
>> +
>> +     /* Shift to get chip number */
>> +     chipnr = (int)(ofs >> chip->chip_shift);
>> +
>> +     /* Select the NAND device */
>> +     chip->select_chip(mtd, chipnr);
>
> This is a similar on, IMHO.

this one too.

>
>> +
>> +     /* Check, if it is write protected */
>> +     if (nand_check_wp(mtd)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
>> +                                     __func__);
>> +             ret = -EIO;
>> +             goto out;
>> +     }
>> +
>> +     ret = __nand_unlock(mtd, ofs, len, 0);
>> +
>> +out:
>> +     /* de-select the NAND device */
>> +     chip->select_chip(mtd, -1);
>> +
>> +     /* Deselect and wake up anyone waiting on the device */
>> +     nand_release_device(mtd);
> And the above 2.
>
> And similar useless comments are in the 'lock' function..

I'll take care of these, everywhere else.

>
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>> + * nand_lock - [REPLACABLE] locks all blockes present in the device
>> + *
>> + * @param mtd - mtd info
>> + * @param ofs - offset to start unlock from
>> + * @param len - length to unlock
>> + *
>> + * @return - lock status
>> + *
>> + * This feature is not support in many NAND parts. 'Micron' NAND parts
>> + * do have this feature, but it allows only to lock all blocks not for
>> + * specified range for block.
>> + *
>> + * Implementing 'lock' feature by making use of 'unlock', for now.
>> + */
>> +static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +{
>> +     int ret = 0;
>> +     int chipnr, status, page;
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> +                     __func__, (unsigned long long)ofs, len);
>> +
>> +     /* Start address must align on block boundary */
>> +     if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Length must align on block boundary */
>> +     if (len & ((1 << chip->phys_erase_shift) - 1)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     /* Do not allow past end of device */
>> +     if (ofs + len > mtd->size) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
>> +                                     __func__);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>
> Repeated pattern, needs a helper function.

same comment as I said before about this.

>
>> +
>> +     /* Grab the lock and see if the device is available */
>> +     nand_get_device(chip, mtd, FL_LOCKING);
>> +
>> +     /* Shift to get first page */
>> +     page = (int)(ofs >> chip->page_shift);
>> +     chipnr = (int)(ofs >> chip->chip_shift);
>> +
>> +     /* Select the NAND device */
>> +     chip->select_chip(mtd, chipnr);
>> +
>> +     /* Check, if it is write protected */
>> +     if (nand_check_wp(mtd)) {
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
>> +                                     __func__);
>> +             status = MTD_ERASE_FAILED;
>> +             ret = -EIO;
>> +             goto out;
>> +     }
>> +
>> +     /* Submit address of first page to unlock */
>> +     page = (int)(ofs >> chip->page_shift);
>> +     chip->cmdfunc(mtd, NAND_CMD_LOCK, -1, page & chip->pagemask);
>> +
>> +     /* Call wait ready function */
>> +     status = chip->waitfunc(mtd, chip);
>> +     udelay(1000);
>> +     /* See if device thinks it succeeded */
>> +     if (status & 0x01) {
>> +             /* There was an error */
>> +             DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
>> +                                     __func__, status);
>> +             ret = -EIO;
>> +             goto out;
>> +     }
>> +
>> +     if (len != -1)
>> +             ret = __nand_unlock(mtd, ofs, len, 0x1);
>> +
>> +out:
>> +     /* de-select the NAND device */
>> +     chip->select_chip(mtd, -1);
>> +
>> +     /* Deselect and wake up anyone waiting on the device */
>> +     nand_release_device(mtd);
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>>   * nand_read_page_raw - [Intern] read raw page data without ecc
>>   * @mtd:     mtd info structure
>>   * @chip:    nand chip info structure
>> @@ -2999,8 +3211,8 @@ int nand_scan_tail(struct mtd_info *mtd)
>>       mtd->read_oob = nand_read_oob;
>>       mtd->write_oob = nand_write_oob;
>>       mtd->sync = nand_sync;
>> -     mtd->lock = NULL;
>> -     mtd->unlock = NULL;
>> +     mtd->lock = nand_lock;
>> +     mtd->unlock = nand_unlock;
>
> What makes you believe it is safe to assign these call-backs here?
>
> AFAICS, this means it will be done for all flashes. Do all of them
> support lock/unlock? I did not investigate this, but I think that
> probably not.

OK. In that case I'll rather not do it here and do it in my specific driver.

--
Thanks,
Vimal

>
>
>>       mtd->suspend = nand_suspend;
>>       mtd->resume = nand_resume;
>>       mtd->block_isbad = nand_block_isbad;
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index ccab9df..698eada 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -82,6 +82,10 @@ extern void nand_wait_ready(struct mtd_info *mtd);
>>  #define NAND_CMD_ERASE2              0xd0
>>  #define NAND_CMD_RESET               0xff
>>
>> +#define NAND_CMD_LOCK                0x2a
>> +#define NAND_CMD_UNLOCK1     0x23
>> +#define NAND_CMD_UNLOCK2     0x24
>> +
>>  /* Extended commands for large page devices */
>>  #define NAND_CMD_READSTART   0x30
>>  #define NAND_CMD_RNDOUTSTART 0xE0
> --
> Best Regards,
> Artem Bityutskiy (Артём Битюцкий)
>
>



-- 
Regards,
Vimal Singh

Re: [PATCH - V2] Add NAND lock/unlock routines

[David Woodhouse]

On Wed, 2010-01-13 at 14:55 +0530, Vimal Singh wrote:
> 
> >> @@ -2999,8 +3211,8 @@ int nand_scan_tail(struct mtd_info *mtd)
> >>       mtd->read_oob = nand_read_oob;
> >>       mtd->write_oob = nand_write_oob;
> >>       mtd->sync = nand_sync;
> >> -     mtd->lock = NULL;
> >> -     mtd->unlock = NULL;
> >> +     mtd->lock = nand_lock;
> >> +     mtd->unlock = nand_unlock;
> >
> > What makes you believe it is safe to assign these call-backs here?
> >
> > AFAICS, this means it will be done for all flashes. Do all of them
> > support lock/unlock? I did not investigate this, but I think that
> > probably not.
> 
> OK. In that case I'll rather not do it here and do it in my specific
> driver.

Hm, I'm not sure that's the best approach. It's a function of the NAND
chip, not the controller. We should detect the presence of the feature
from the chip ID, if at all possible.

I'm happy with your first two patches as part of a three-patch set
though. Thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH - V2] Add NAND lock/unlock routines

[Vimal Singh]

On Fri, Feb 26, 2010 at 6:17 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2010-01-13 at 14:55 +0530, Vimal Singh wrote:
>>
>> >> @@ -2999,8 +3211,8 @@ int nand_scan_tail(struct mtd_info *mtd)
>> >>       mtd->read_oob = nand_read_oob;
>> >>       mtd->write_oob = nand_write_oob;
>> >>       mtd->sync = nand_sync;
>> >> -     mtd->lock = NULL;
>> >> -     mtd->unlock = NULL;
>> >> +     mtd->lock = nand_lock;
>> >> +     mtd->unlock = nand_unlock;
>> >
>> > What makes you believe it is safe to assign these call-backs here?
>> >
>> > AFAICS, this means it will be done for all flashes. Do all of them
>> > support lock/unlock? I did not investigate this, but I think that
>> > probably not.
>>
>> OK. In that case I'll rather not do it here and do it in my specific
>> driver.
>
> Hm, I'm not sure that's the best approach. It's a function of the NAND
> chip, not the controller. We should detect the presence of the feature
> from the chip ID, if at all possible.
>
> I'm happy with your first two patches as part of a three-patch set
> though. Thanks.
>
Thanks David. I'll see the datasheet if chip ID has this information.

-- 
Regards,
Vimal Singh