[ANNOUNCE,MTD] Flex-OneNAND MTD Driver available.

Hi Adrian,

Adrian Hunter wrote:
>> +static unsigned flexonenand_get_block(struct onenand_chip *this, loff_t addr,
>> +			   unsigned *isblkslc)
>> +{
>> +	unsigned boundary, blk, die = 0;
>> +
>> +	if (unlikely(this->chipsize == 0))
>> +		/* We have been called by flexonenand_get_boundary.
>> +		 * addr contains die index in this case.
>> +		 */
> 
> This seems to affect only FLEXONENAND_CMD_PI_ACCESS and ONENAND_CMD_READ.
> onenand_command() can be amended to handle FLEXONENAND_CMD_PI_ACCESS,
> and a fake command could be added for the ONENAND_CMD_READ case,
> then this kludge goes away.

Thanks for the suggestion. Fixed.

>> +	/* check if we failed due to uncorrectable error */
>> +	if (status != (-EBADMSG) && status != (ONENAND_BBT_READ_ECC_ERROR))
> 
> Please omit the unnecessary parentheses around -EBADMSG and ONENAND_BBT_READ_ECC_ERROR.

Fixed.

>> +	/* We are attempting to reread, so decrement stats.failed
>> +	 * which was incremented by onenand_wait due to read failure
>> +	 */
>> +	printk(KERN_INFO "Attempting to recover from uncorrectable read\n");
> 
> Message with no source. How about: "onenand_recover_lsb: Attempting to recover from uncorrectable read\n"

Fixed.

>> +	/* Do not allow reads past end of device */
>> +	if ((from + len) > mtd->size) {
> 
> Please omit unnecessary parenthesis

Fixed.

>> +			ret = this->wait(mtd, FL_READING);
>> +			ret = unlikely(ret) ? onenand_recover_lsb(mtd, from, ret) : ret;
> 
> That is unusual syntax.  The following is much more common:

Fixed

>> +	for (die = 0; die < this->dies; die++) {
>> +		this->command(mtd, FLEXONENAND_CMD_PI_ACCESS, die, 0);
>> +		this->wait(mtd, FL_SYNCING);
> 
> Why is the error return not checked?
> 
>> +
>> +		this->command(mtd, ONENAND_CMD_READ, die, 0);
>> +		ret = this->wait(mtd, FL_READING);
> 
> Why is the error return not checked?
> 
>> +
>> +		bdry = this->read_word(this->base + ONENAND_DATARAM);
>> +		locked = bdry >> FLEXONENAND_PI_UNLOCK_SHIFT;
>> +		locked = (locked == 0x3) ? 0 : 1;
>> +		this->boundary[die] = bdry & FLEXONENAND_PI_MASK;
>> +		this->boundary_locked[die] = locked;
>> +		this->command(mtd, ONENAND_CMD_RESET, 0, 0);
>> +		ret = this->wait(mtd, FL_RESETING);
> 
> Why is the error return not checked?

As per datasheet PI_ACCESS, PI_UPDATE, PI_READ and RESET always succeed.
PI_ERASE and PI_WRITE can fail. Added error check for PI_ERASE.

>> +		printk(KERN_ERR "Failed PI update for Die %d\n", die);
>> +	else
>> +		printk(KERN_INFO "Done\n");
>> +out:
>> +	this->write_word(ONENAND_CMD_RESET, this->base + ONENAND_REG_COMMAND);
>> +	this->wait(mtd, FL_RESETING);
>> +	if (!ret)
>> +		/* Recalculate device size on boundary change*/
>> +		flexonenand_get_size(mtd);
>> +	return ret;
>> +}
> 
> The functions above often use 'printk()' but do not identify the message source.
> I suggest adding the function name e.g. "flexonenand_set_boundary: blah blah"
> 
> Also using line continuations for strings results in the string
> containing unwanted tabs and spaces. e.g.
> 
> 				"Invalid boundary value.\
> 					 Boundary not changed.\n"
> 
> results in:
> 
> "Invalid boundary value.					 Boundary not changed.\n"
> 
> Instead you can just close and reopen the string e.g.
> 				"Invalid boundary value."
> 					" Boundary not changed.\n"

Fixed

>> +	if (FLEXONENAND(this)) {
>> +		int boundary[] =
>> +			{FLEXONENAND_DIE0_BOUNDARY, FLEXONENAND_DIE1_BOUNDARY};
>> +		int lock[] =
>> +			{FLEXONENAND_DIE0_ISLOCKED, FLEXONENAND_DIE1_ISLOCKED};
>> +		unsigned die;
>> +
>> +		flexonenand_get_size(mtd);
>> +
>> +		/* Change the device boundaries if required */
>> +		for (die = 0; die < this->dies; die++)
>> +			if ((!this->boundary_locked[die]) &&
>> +			   (boundary[die] >= 0) &&
>> +			   (boundary[die] != this->boundary[die]))
>> +				flexonenand_set_boundary(mtd, die,
>> +						 boundary[die], lock[die]);
> 
> I am not sure how you intend FLEXONENAND_DIEx_BOUNDARY and FLEXONENAND_DIEx_ISLOCKED
> to be used.  Obviously this code presently does nothing.  If you expect the values
> to be configured for the device, then shouldn't they be config options?

Ok. Added config option for boundary and lock settings.
Configuration of boundary / lock is optional. Default value in config is -1
which will not change existing boundary of Flex-OneNAND.

> 
> Also, ideally there should be a way for the platform driver to override them.
> 

Do you mean runtime configuration of boundary. Then we need to add some ioctl.

>> +	if (FLEXONENAND(this))
>> +		kfree(mtd->eraseregions);
> 
> You can drop the "if (FLEXONENAND(this))", since mtd->eraseregions will
> be NULL if it hasn't been allocated.

ok

>> +		if (FLEXONENAND(this)) {
>> +			onenand_get_block(this, from, &slc);
>> +			from += (1 << bbm->bbt_erase_shift) >> 1;
>> +			if (!slc)
>> +				from += (1 << bbm->bbt_erase_shift) >> 1;
> 
> I think the following syntax is clearer:
> 
> 			if (slc)
> 				from += (1 << bbm->bbt_erase_shift) >> 1;
> 			else
> 				from += (1 << bbm->bbt_erase_shift);

ok. Fixed.

>>   * @chipsize:		[INTERN] the size of one chip for multichip arrays
> 
> 'chipsize' was never properly implemented for multichip devices (it was always
> the same as mtd->size) and it now has another meaning.  For Flex-OneNAND, it is
> the maximum device size.  Please update the comments accordingly.  It is really
> a FIXME.

For now it is FIXME. will fix it

Also, onenand_erase is more readable now.

Thanks,
Rohit

 drivers/mtd/onenand/Kconfig        |   62 +++
 drivers/mtd/onenand/onenand_base.c |  735 +++++++++++++++++++++++++++++++++----
 drivers/mtd/onenand/onenand_bbt.c  |   13 
 drivers/mtd/onenand/onenand_sim.c  |  105 ++++-
 include/linux/mtd/onenand.h        |   34 +
 include/linux/mtd/onenand_regs.h   |   20 -
 6 files changed, 883 insertions(+), 86 deletions(-)

Signed-off-by: Vishak G <vishak.g@samsung.com>
Signed-off-by: Rohit Hagargundgi <h.rohit@samsung.com>
---
---

[ANNOUNCE,MTD] Flex-OneNAND MTD Driver available.

Commit Message

Comments

Patch