Patchwork [RFC] Add NAND lock/unlock routines

login
register
mail settings
Submitter vimal singh
Date Dec. 2, 2009, 2:24 p.m.
Message ID <ce9ab5790912020624k28bc1e63le0f7e713f70d78a8@mail.gmail.com>
Download mbox | patch
Permalink /patch/40044/
State New
Headers show

Comments

vimal singh - Dec. 2, 2009, 2:24 p.m.
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(-)
Artem Bityutskiy - Dec. 4, 2009, 8:38 a.m.
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?
vimal singh - Dec. 7, 2009, 6:48 a.m.
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.
Artem Bityutskiy - Dec. 7, 2009, 9:29 a.m.
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?
vimal singh - Dec. 7, 2009, 11:06 a.m.
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.
Artem Bityutskiy - Dec. 7, 2009, 11:28 a.m.
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.
vimal singh - Dec. 17, 2009, 9:41 a.m.
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?
Artem Bityutskiy - Jan. 7, 2010, 6:53 a.m.
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.

Patch

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 */