[PATCH - V2] Add NAND lock/unlock routines
Vimal Singh
vimal.newwork at gmail.com
Wed Jan 13 04:25:54 EST 2010
On Wed, Jan 13, 2010 at 2:10 PM, Artem Bityutskiy <dedekind1 at 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 at 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
More information about the linux-mtd
mailing list