[PATCH] OneNAND MTD support

[Kyungmin Park]

Hi 

Thank you for comments and advice.

Here's new patch.

Regards

Kyungmin Park

> > > +static int onenand_unlock(struct mtd_info *mtd, loff_t ofs, 
> > There's no status check in locking mechanism
> 
> Hmm. So all the status errors checked in onenand_wait() will never
> happen here ?
> 
> > > 
> > > +		/* Sanity check */
> > > +		while (onenand_readw(this->base + 
> > > ONENAND_REG_CTRL_STATUS)
> > > +		    & ONENAND_CTRL_ONGO)
> > > +			continue;
> > > 
> > > What happens if the condition is not met ?
> > 
> > I think it's board bug. For safety I check it. 
> 
> Then some kind of timeout is definitely appropriate. You will 
> endup in a
> dead loop when the condition is not met due to the bug.

In OneNAND after operation done, The interrupt and controller status is
updated simultaneously.
So it's no problem checking controller status.
If the problem occurs, It's chip bug.

> 
> > > +/*
> > > + * Functions to access the OneNAND IO region
> > > + */
> > > +#define onenand_readw(a)		readw(a)
> > > +#define onenand_writew(v,a)		writew(v, a)
> > > 
> > > Please make onenand_readw/writew functions accessible by function
> > > pointers along with default implementations, so the board 
> driver can
> > > override them if necessary.
> > > 
> > 
> > It's just memory access function. I think no need to make a 
> function.
> 
> Yes, on your board. It turned out to be a good choice to turn 
> _all_ the
> functions which access hardware into function pointers. People design
> weird hardware and the overhead of the function pointer based 
> access is
> minimal. Keeping it your way, one has to replace 4 complex functions
> (command, wait, read/ write) instead of providing 4 small and simple
> ones.

OK. I do it

> 
> +config MTD_ONENAND_OMAP
> +	tristate "OneNAND Flash device on OMAP board"
> +	depends on ARM && ARCH_OMAP && MTD_ONENAND
> 
> "depends on ARCH_OMAP && MTD_ONENAND" is enough, as ARCH_OMAP already
> depends on ARM

It's derived from omap-nand-flash.c, But it's not sended to MTD.
OK. I done

> 
> 
> +++ uncommitted/drivers/mtd/onenand/omap-onenand.c  
> +	default:
> +		printk(KERN_WARNING "Unsupported OneNAND device\n");
> +		err = -ENXIO;
> +		goto out_buf;
> +	}
> +
> +	goto out;
> 
> 	return 0; 
> This is the normal return path. Below is error exit (cleanup)

OK

> 
> +++ uncommitted/drivers/mtd/onenand/onenand_base.c  (mode:100644)
> +static int onenand_read_bufferram(struct mtd_info *mtd, int area,
> +		unsigned char *buffer, int offset, size_t count)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +	void __iomem *bufferram;
> +
> +	if (area == DATARAM)
> +		bufferram = this->base + ONENAND_DATARAM;
> +	else if (area == SPARERAM)
> +		bufferram = this->base + ONENAND_SPARERAM;
> +	else  
> +		bufferram = this->base + ONENAND_BOOTRAM;
> +
> +	if (ONENAND_CURRENT_BUFFERRAM(this)) {
> +		if (area == DATARAM)
> +			bufferram += mtd->oobblock;
> +		else if (area == SPARERAM)
> +			bufferram += mtd->oobsize;
> +	}
> +
> +	memcpy(buffer, bufferram + offset, count);
> +
> +	return 0;
> +}
> 
> area can be a direct offset (ONENAND_DATARAM, ONENAND_SPARERAM,
> ONENAND_BOOTRAM), so you can just write: bufferram = 
> this->base + area;
> 
> 
> +static int onenand_write_bufferram(struct mtd_info *mtd, int area,
> +		const unsigned char *buffer, int offset, size_t count)
> +{
> 
> Same as onenand_read_bufferram

Yes It is more natual. why I don't know this method :)

> 
> +	if (ONENAND_CURRENT_BUFFERRAM(this)) {
> +		if (area == DATARAM)
> +			bufferram += mtd->oobblock;
> +		else if (area == SPARERAM)
> +			bufferram += mtd->oobsize;
> +	}
> 
> Please make this a inline function used in read/write_bufferram

I done.

> 
> +EXPORT_SYMBOL(onenand_scan);
> +EXPORT_SYMBOL(onenand_release);
> 
> Would you mind to change those exports to EXPORT_SYMBOL_GPL ?

In fact I not sure and also don't know license issue. 
If we develop the some commercial mtd user software then can we use mtd
device code exported with GPL? Is there no problem?

> 
> +++ uncommitted/include/linux/mtd/onenand_regs.h  (mode:100644)
> 
> +/* Memory Address Map Translation (Word order) */
> +#define ONENAND_MEMORY_MAP(x)		((x) << 1)
> 
> Is this considered to be a general rule, i.e. is this the 
> only valid way
> to address the chip ? 

To comply with Spec. In Spec. most register addresses are written in word
e.g., Interrupt status registers is 0xf241h
#define ONENAND_REG_INTERRUPT          ONENAND_MEMORY_MAP(0xF241)

> 
> +struct onenand_chip {
> +	void __iomem		*base;
> +	unsigned int		chipsize;
> +	unsigned int		device_id;
> +	unsigned int		version_id;
> +	unsigned int		options;
> 
> Are device_id and version_id necessary for anything else than 
> scan ? If
> no, please remove.
> 

device_id necessary to control DDP chip. But I'm not yet tested with DDP
chip.
version_id is not necessary. removed. 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: onenand-mtd.patch
Type: application/octet-stream
Size: 57742 bytes
Desc: not available
Url : http://lists.infradead.org/pipermail/linux-mtd/attachments/20050711/ca7e5350/attachment.obj