Re: [PATCH] mtd/nand: don't use {read,write}_buf for 8-bit transfers

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello Huang Shijie (is this the right name to use in a greeting?),

On Fri, Mar 01, 2013 at 11:34:27AM +0800, Huang Shijie wrote:
> 于 2013年02月28日 18:48, Uwe Kleine-König 写道:
> >On Thu, Feb 28, 2013 at 10:47:43AM +0800, Huang Shijie wrote:
> >>于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> >>>According to the Open NAND Flash Interface Specification (ONFI) Revision
> >>>3.1 "Parameters are always transferred on the lower 8-bits of the data
> >>>bus." for the Get Features and Set Features commands.
> >>>
> >>yes. the set/get features should works in 8-bit.
> >>
> >>I have never met a 16-bit onfi nand yet. :)
> >>
> >>>So using read_buf and write_buf is wrong for 16-bit wide nand chips as
> >>>they use I/O[15:0]. The Get Features command is easily fixed using 4
> >>>times the read_byte callback. For Set Features error out as there is no
> >>yes. for get features, it's easy to fix it.
> >>>write_byte callback.
> >>Most of the time, the nand controller will overwrite the write_buf hook...
> >>I also think we need a write_byte callback.
> >a default implementation could be something like that:
> >
> >	static void nand_write_byte(struct mtd_info *mtd, uint8_t byte)
> >	{
> >		struct nand_chip *chip = mtd->priv;
> >
> >		if (chip->options&  NAND_BUSWIDTH_16)
> >			chip->write_buf(mtd, (uint8_t[]){ byte, 0 }, 2);
> >		else
> >			chip->write_buf(mtd,&byte, 1);
> >	}
> >
> >(Is this the correct order in the array? Or might that depend on
> >endianess?)
> >
> >Does this look right?
> >
> IMHO, the nand_write_byte() should not call the chip->write_buf()
> again. Since the ->write_buf() could
> be the nand_write_buf16(). it makes a little mess.
I think it does the right thing though. With a 16-bit chip doing

	chip->write_buf(mtd, (uint8_t[]){ byte, 0 }, 2)

puts $byte to I/O[7:0] and 0 to I/O[15:8]. This is what I want it to
do---not sure if I/O[15:8] should better be tri-stated?

> In my opinion, the default nand_write_byte() hook could use the
> nand_write_buf() to  write just one byte;
This feels much more wrong. nand_write_buf() uses chip->IO_ADDR_W which
might not be initialized by the driver.

> and the nand controller can overwrite the nand_write_byte() hook if
> it could.
> Of course, it's just a suggest.
I will create a patch ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |