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

[Artem Bityutskiy <dedekind1@gmail.com>]

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 (Артём Битюцкий)