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

[Brian Norris]

(Huang: a few comments are directed at you below)

Hi Uwe,

On Thu, Dec 05, 2013 at 10:22:04PM +0100, Uwe Kleine-König wrote:
> 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.
> 
> 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 implement a new
> overwritable callback "write_byte". Still I expect the default to work
> just fine for all controllers and making it overwriteable was just done
> for symmetry.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> ---
> Changes since v2, sent with
> Message-Id: 1362415655-13073-1-git-send-email-u.kleine-koenig at pengutronix.de:
> 
>  - use a proper uint16_t instead of a 2 byte array to pass to chip->write_buf
>    to fix endianess issue. Noticed by David Woodhouse. (This also gets rid of
>    the Compound Literal that catched David's eye.)
> 
> Changes since v3, sent with
> Message-id:1385500515-5376-1-git-send-email-u.kleine-koenig at pengutronix.de (v3)
> 
>  - drop whitespace cleanup
>  - support resetting to 16 bit width function on 2nd call of nand_set_defaults

Thanks for the changes. This patch looks good to me, and I think it can
go in regardless of our other x8/x16 buswidth solutions.

I don't have a good x16 setup yet, but I believe Ezequiel did some
testing on v3 (is this a "Tested-by"?). I'll probably try this out on my
x8 setups when I get a chance sometime this week.

>  drivers/mtd/nand/nand_base.c | 57 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/nand.h     |  3 +++
>  2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b..3a6e95f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -202,6 +202,51 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr)
>  }
>  
>  /**
> + * nand_write_byte - [DEFAULT] write single byte to chip
> + * @mtd: MTD device structure
> + * @byte: value to write
> + *
> + * Default function to write a byte to I/O[7:0]
> + */
> +static void nand_write_byte(struct mtd_info *mtd, uint8_t byte)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	chip->write_buf(mtd, &byte, 1);

Side issue: Huang, this is another potential pitfall for GPMI, as you
will again be performing DMA on the stack.

> +}
> +
> +/**
> + * nand_write_byte16 - [DEFAULT] write single byte to a chip with width 16
> + * @mtd: MTD device structure
> + * @byte: value to write
> + *
> + * Default function to write a byte to I/O[7:0] on a 16-bit wide chip.
> + */
> +static void nand_write_byte16(struct mtd_info *mtd, uint8_t byte)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	uint16_t word = byte;
> +
> +	/*
> +	 * It's not entirely clear what should happen to I/O[15:8] when writing
> +	 * a byte. The ONFi spec (Revision 3.1; 2012-09-19, Section 2.16) reads:
> +	 *
> +	 *    When the host supports a 16-bit bus width, only data is
> +	 *    transferred at the 16-bit width. All address and command line
> +	 *    transfers shall use only the lower 8-bits of the data bus. During
> +	 *    command transfers, the host may place any value on the upper
> +	 *    8-bits of the data bus. During address transfers, the host shall
> +	 *    set the upper 8-bits of the data bus to 00h.
> +	 *
> +	 * One user of the write_byte callback is nand_onfi_set_features. The
> +	 * four parameters are specified to be written to I/O[7:0], but this is
> +	 * neither an address nor a command transfer. Let's assume a 0 on the
> +	 * upper I/O lines is OK.
> +	 */
> +	chip->write_buf(mtd, &word, 2);

Huang: same here. So you see how this type of patch can easily trigger
the maintainability problems that are inherent when gpmi-nand does
things differently than other drivers?

> +}
> +
> +/**
>   * nand_write_buf - [DEFAULT] write buffer to chip
>   * @mtd: MTD device structure
>   * @buf: data buffer

Brian