[PATCH] mtdchar: handle chips that have user otp but no factory otp

[PATCH] mtdchar: handle chips that have user otp but no factory otp

[Uwe Kleine-König]

Before this patch mtd_read_fact_prot_reg was used to check availability
for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
user otp for chips that don't have a factory otp area impossible. So use
the right wrapper depending on the intended area to be accessed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I'm currently working on accessing the user otp area of MT29F2G nand
flash that doesn't have a factory otp. Reading and writing are not ready
yet, but this change is an obvious prerequisite.

Best regards
Uwe

 drivers/mtd/mtdchar.c |   30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a6e7451..1f4034a 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -371,26 +371,34 @@ static int otp_select_filemode(struct mtd_file_info *mfi, int mode)
 	struct mtd_info *mtd = mfi->mtd;
 	size_t retlen;
 	int ret = 0;
-
-	/*
-	 * Make a fake call to mtd_read_fact_prot_reg() to check if OTP
-	 * operations are supported.
-	 */
-	if (mtd_read_fact_prot_reg(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
-		return -EOPNOTSUPP;
+	int (*func)(struct mtd_info *, loff_t, size_t, size_t *, u_char *) =
+		NULL;
+	enum mtd_file_modes outmode = MTD_FILE_MODE_NORMAL;
 
 	switch (mode) {
 	case MTD_OTP_FACTORY:
-		mfi->mode = MTD_FILE_MODE_OTP_FACTORY;
+		outmode = MTD_FILE_MODE_OTP_FACTORY;
+		func = mtd_read_fact_prot_reg;
 		break;
 	case MTD_OTP_USER:
-		mfi->mode = MTD_FILE_MODE_OTP_USER;
+		outmode = MTD_FILE_MODE_OTP_USER;
+		func = mtd_read_user_prot_reg;
 		break;
-	default:
-		ret = -EINVAL;
 	case MTD_OTP_OFF:
 		break;
+	default:
+		ret = -EINVAL;
 	}
+
+	/*
+	 * Make a fake call to mtd_read_fact_prot_reg() or
+	 * mtd_read_user_prot_reg() to check if OTP operations are supported.
+	 */
+	if (func && func(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
+		return -EOPNOTSUPP;
+
+	mfi->mode = outmode;
+
 	return ret;
 }
 #else
-- 
1.7.10.4

Re: [PATCH] mtdchar: handle chips that have user otp but no factory otp

[Artem Bityutskiy]

On Mon, 2013-02-18 at 11:19 +0100, Uwe Kleine-König wrote:
> Before this patch mtd_read_fact_prot_reg was used to check availability
> for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
> user otp for chips that don't have a factory otp area impossible. So use
> the right wrapper depending on the intended area to be accessed.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I'm currently working on accessing the user otp area of MT29F2G nand
> flash that doesn't have a factory otp. Reading and writing are not ready
> yet, but this change is an obvious prerequisite.
> 
> Best regards
> Uwe
> 
>  drivers/mtd/mtdchar.c |   30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index a6e7451..1f4034a 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -371,26 +371,34 @@ static int otp_select_filemode(struct mtd_file_info *mfi, int mode)
>  	struct mtd_info *mtd = mfi->mtd;
>  	size_t retlen;
>  	int ret = 0;
> -
> -	/*
> -	 * Make a fake call to mtd_read_fact_prot_reg() to check if OTP
> -	 * operations are supported.
> -	 */
> -	if (mtd_read_fact_prot_reg(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
> -		return -EOPNOTSUPP;
> +	int (*func)(struct mtd_info *, loff_t, size_t, size_t *, u_char *) =
> +		NULL;
> +	enum mtd_file_modes outmode = MTD_FILE_MODE_NORMAL;

In this particular case when we only have 2 modes I'd prefer to not
introduce a function pointer variable but directly call the function.
>  
>  	switch (mode) {
>  	case MTD_OTP_FACTORY:
> -		mfi->mode = MTD_FILE_MODE_OTP_FACTORY;
> +		outmode = MTD_FILE_MODE_OTP_FACTORY;
> +		func = mtd_read_fact_prot_reg;
>  		break;
>  	case MTD_OTP_USER:
> -		mfi->mode = MTD_FILE_MODE_OTP_USER;
> +		outmode = MTD_FILE_MODE_OTP_USER;
> +		func = mtd_read_user_prot_reg;
>  		break;
> -	default:
> -		ret = -EINVAL;
>  	case MTD_OTP_OFF:
>  		break;
> +	default:
> +		ret = -EINVAL;

Probably in this case you need to 'return -EINVAL', otherwise you modify
'mfi->mode' below which is an unexpected side-effect of '-EINVAL'.
>  	}
> +
> +	/*
> +	 * Make a fake call to mtd_read_fact_prot_reg() or
> +	 * mtd_read_user_prot_reg() to check if OTP operations are supported.
> +	 */
> +	if (func && func(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
> +		return -EOPNOTSUPP;
> +
> +	mfi->mode = outmode;
> +
>  	return ret;
>  }
>  #else

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] mtdchar: handle chips that have user otp but no factory otp

[Uwe Kleine-König]

Hello,

On Sat, Mar 02, 2013 at 05:41:24PM +0200, Artem Bityutskiy wrote:
> On Mon, 2013-02-18 at 11:19 +0100, Uwe Kleine-König wrote:
> > Before this patch mtd_read_fact_prot_reg was used to check availability
> > for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
> > user otp for chips that don't have a factory otp area impossible. So use
> > the right wrapper depending on the intended area to be accessed.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > I'm currently working on accessing the user otp area of MT29F2G nand
> > flash that doesn't have a factory otp. Reading and writing are not ready
> > yet, but this change is an obvious prerequisite.
> > 
> > Best regards
> > Uwe
> > 
> >  drivers/mtd/mtdchar.c |   30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> > index a6e7451..1f4034a 100644
> > --- a/drivers/mtd/mtdchar.c
> > +++ b/drivers/mtd/mtdchar.c
> > @@ -371,26 +371,34 @@ static int otp_select_filemode(struct mtd_file_info *mfi, int mode)
> >  	struct mtd_info *mtd = mfi->mtd;
> >  	size_t retlen;
> >  	int ret = 0;
> > -
> > -	/*
> > -	 * Make a fake call to mtd_read_fact_prot_reg() to check if OTP
> > -	 * operations are supported.
> > -	 */
> > -	if (mtd_read_fact_prot_reg(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
> > -		return -EOPNOTSUPP;
> > +	int (*func)(struct mtd_info *, loff_t, size_t, size_t *, u_char *) =
> > +		NULL;
> > +	enum mtd_file_modes outmode = MTD_FILE_MODE_NORMAL;
> 
> In this particular case when we only have 2 modes I'd prefer to not
> introduce a function pointer variable but directly call the function.
I choosed the function pointer to reduce code duplication. Do function
pointer have a bad characteristic that I don't know? But actually I
don't care much and can resend without the function pointer.
> >  
> >  	switch (mode) {
> >  	case MTD_OTP_FACTORY:
> > -		mfi->mode = MTD_FILE_MODE_OTP_FACTORY;
> > +		outmode = MTD_FILE_MODE_OTP_FACTORY;
> > +		func = mtd_read_fact_prot_reg;
> >  		break;
> >  	case MTD_OTP_USER:
> > -		mfi->mode = MTD_FILE_MODE_OTP_USER;
> > +		outmode = MTD_FILE_MODE_OTP_USER;
> > +		func = mtd_read_user_prot_reg;
> >  		break;
> > -	default:
> > -		ret = -EINVAL;
> >  	case MTD_OTP_OFF:
> >  		break;
> > +	default:
> > +		ret = -EINVAL;
> 
> Probably in this case you need to 'return -EINVAL', otherwise you modify
> 'mfi->mode' below which is an unexpected side-effect of '-EINVAL'.
I don't remember, but I think I convinced myself that it doesn't hurt.
Still it's less surprising to not modify mfi, so OK.

Uwe

> >  	}
> > +
> > +	/*
> > +	 * Make a fake call to mtd_read_fact_prot_reg() or
> > +	 * mtd_read_user_prot_reg() to check if OTP operations are supported.
> > +	 */
> > +	if (func && func(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
> > +		return -EOPNOTSUPP;
> > +
> > +	mfi->mode = outmode;
> > +
> >  	return ret;
> >  }
> >  #else
> 
> -- 
> Best Regards,
> Artem Bityutskiy
> 
> 

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

[PATCH v2] mtdchar: handle chips that have user otp but no factory otp

[Uwe Kleine-König]

Before this patch mtd_read_fact_prot_reg was used to check availability
for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
user otp for chips that don't have a factory otp area impossible. So use
the right wrapper depending on the intended area to be accessed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Forwarded: id:1361182768-31919-1-git-send-email-u.kleine-koenig@pengutronix.de
---

Notes:
    Changes since (implicit) v1, sent with
    Message-Id:1361182768-31919-1-git-send-email-u.kleine-koenig@pengutronix.de:
    
     - drop usage of a function pointer and accept minimal code duplication
       instead;
     - don't modify mfi->mode when returning -EINVAL;

 drivers/mtd/mtdchar.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 82c0616..68959a3 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -370,28 +370,30 @@ static int otp_select_filemode(struct mtd_file_info *mfi, int mode)
 {
 	struct mtd_info *mtd = mfi->mtd;
 	size_t retlen;
-	int ret = 0;
-
-	/*
-	 * Make a fake call to mtd_read_fact_prot_reg() to check if OTP
-	 * operations are supported.
-	 */
-	if (mtd_read_fact_prot_reg(mtd, -1, 0, &retlen, NULL) == -EOPNOTSUPP)
-		return -EOPNOTSUPP;
 
 	switch (mode) {
 	case MTD_OTP_FACTORY:
+		if (mtd_read_fact_prot_reg(mtd, -1, 0, &retlen, NULL) ==
+				-EOPNOTSUPP)
+			return -EOPNOTSUPP;
+
 		mfi->mode = MTD_FILE_MODE_OTP_FACTORY;
 		break;
 	case MTD_OTP_USER:
+		if (mtd_read_user_prot_reg(mtd, -1, 0, &retlen, NULL) ==
+				-EOPNOTSUPP)
+			return -EOPNOTSUPP;
+
 		mfi->mode = MTD_FILE_MODE_OTP_USER;
 		break;
-	default:
-		ret = -EINVAL;
 	case MTD_OTP_OFF:
+		mfi->mode = MTD_FILE_MODE_NORMAL;
 		break;
+	default:
+		return -EINVAL;
 	}
-	return ret;
+
+	return 0;
 }
 #else
 # define otp_select_filemode(f,m)	-EOPNOTSUPP
-- 
1.8.2.rc2

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

[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.

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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---

Notes:
    v1 got an
    	Acked-by: Huang Shijie <b32955@freescale.com>
    
    Changes since (implicit) v1, sent with
    Message-Id:1361977852-18233-1-git-send-email-u.kleine-koenig@pengutronix.de:
    
     - implement a new write_byte callback to also fix
       nand_onfi_set_features (instead of returning -ENOSUPP in v1)

 drivers/mtd/nand/nand_base.c | 60 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/mtd/nand.h     |  3 +++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 3766682..956b499 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -205,6 +205,50 @@ 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);
+}
+
+/**
+ * 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;
+
+	/*
+	 * 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, (uint8_t[]){ byte, 0 }, 2);
+}
+
+/**
  * nand_write_buf - [DEFAULT] write buffer to chip
  * @mtd: MTD device structure
  * @buf: data buffer
@@ -2710,7 +2754,7 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
- * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
+ * nand_onfi_set_features - [REPLACEABLE] set features for ONFI nand
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
@@ -2720,12 +2764,15 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
 	int status;
+	int i;
 
 	if (!chip->onfi_version)
 		return -EINVAL;
 
 	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
-	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		chip->write_byte(mtd, subfeature_param[i]);
+
 	status = chip->waitfunc(mtd, chip);
 	if (status & NAND_STATUS_FAIL)
 		return -EIO;
@@ -2733,7 +2780,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
- * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
+ * nand_onfi_get_features - [REPLACEABLE] get features for ONFI nand
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
@@ -2742,6 +2789,8 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
+	int i;
+
 	if (!chip->onfi_version)
 		return -EINVAL;
 
@@ -2749,7 +2798,8 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 	memset(subfeature_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
 
 	chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES, addr, -1);
-	chip->read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		*subfeature_param++ = chip->read_byte(mtd);
 	return 0;
 }
 
@@ -2804,6 +2854,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 		chip->block_markbad = nand_default_block_markbad;
 	if (!chip->write_buf)
 		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
+	if (!chip->write_byte)
+		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
 	if (!chip->read_buf)
 		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
 	if (!chip->scan_bbt)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7ccb3c5..8ee4ed1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -419,6 +419,8 @@ struct nand_buffers {
  *			flash device.
  * @read_byte:		[REPLACEABLE] read one byte from the chip
  * @read_word:		[REPLACEABLE] read one word from the chip
+ * @write_byte		[REPLACEABLE] write a single byte to the chip on the
+ *			low 8 I/O lines
  * @write_buf:		[REPLACEABLE] write data from the buffer to the chip
  * @read_buf:		[REPLACEABLE] read data from the chip into the buffer
  * @select_chip:	[REPLACEABLE] select chip nr
@@ -503,6 +505,7 @@ struct nand_chip {
 
 	uint8_t (*read_byte)(struct mtd_info *mtd);
 	u16 (*read_word)(struct mtd_info *mtd);
+	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
 	void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
 	void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
 	void (*select_chip)(struct mtd_info *mtd, int chip);
-- 
1.8.2.rc2

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

[Uwe Kleine-König]

Hello,

I forgot to add Huang Shijie to Cc: and copied the wrong Message-Id for
the --in-reply-to parameter to git-send-email.

@Huang Shijie: I didn't carry over your Ack to this new patch. Are you
still happy with this one?

Sorry,
Uwe

On Mon, Mar 04, 2013 at 05:47:35PM +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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> 
> Notes:
>     v1 got an
>     	Acked-by: Huang Shijie <b32955@freescale.com>
>     
>     Changes since (implicit) v1, sent with
>     Message-Id:1361977852-18233-1-git-send-email-u.kleine-koenig@pengutronix.de:
>     
>      - implement a new write_byte callback to also fix
>        nand_onfi_set_features (instead of returning -ENOSUPP in v1)
> 
>  drivers/mtd/nand/nand_base.c | 60 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/mtd/nand.h     |  3 +++
>  2 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 3766682..956b499 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -205,6 +205,50 @@ 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);
> +}
> +
> +/**
> + * 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;
> +
> +	/*
> +	 * 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, (uint8_t[]){ byte, 0 }, 2);
> +}
> +
> +/**
>   * nand_write_buf - [DEFAULT] write buffer to chip
>   * @mtd: MTD device structure
>   * @buf: data buffer
> @@ -2710,7 +2754,7 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  }
>  
>  /**
> - * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
> + * nand_onfi_set_features - [REPLACEABLE] set features for ONFI nand
>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
> @@ -2720,12 +2764,15 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
>  			int addr, uint8_t *subfeature_param)
>  {
>  	int status;
> +	int i;
>  
>  	if (!chip->onfi_version)
>  		return -EINVAL;
>  
>  	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
> -	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
> +	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> +		chip->write_byte(mtd, subfeature_param[i]);
> +
>  	status = chip->waitfunc(mtd, chip);
>  	if (status & NAND_STATUS_FAIL)
>  		return -EIO;
> @@ -2733,7 +2780,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
>  }
>  
>  /**
> - * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
> + * nand_onfi_get_features - [REPLACEABLE] get features for ONFI nand
>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
> @@ -2742,6 +2789,8 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
>  static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
>  			int addr, uint8_t *subfeature_param)
>  {
> +	int i;
> +
>  	if (!chip->onfi_version)
>  		return -EINVAL;
>  
> @@ -2749,7 +2798,8 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
>  	memset(subfeature_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
>  
>  	chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES, addr, -1);
> -	chip->read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
> +	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> +		*subfeature_param++ = chip->read_byte(mtd);
>  	return 0;
>  }
>  
> @@ -2804,6 +2854,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
>  		chip->block_markbad = nand_default_block_markbad;
>  	if (!chip->write_buf)
>  		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
> +	if (!chip->write_byte)
> +		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
>  	if (!chip->read_buf)
>  		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
>  	if (!chip->scan_bbt)
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 7ccb3c5..8ee4ed1 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -419,6 +419,8 @@ struct nand_buffers {
>   *			flash device.
>   * @read_byte:		[REPLACEABLE] read one byte from the chip
>   * @read_word:		[REPLACEABLE] read one word from the chip
> + * @write_byte		[REPLACEABLE] write a single byte to the chip on the
> + *			low 8 I/O lines
>   * @write_buf:		[REPLACEABLE] write data from the buffer to the chip
>   * @read_buf:		[REPLACEABLE] read data from the chip into the buffer
>   * @select_chip:	[REPLACEABLE] select chip nr
> @@ -503,6 +505,7 @@ struct nand_chip {
>  
>  	uint8_t (*read_byte)(struct mtd_info *mtd);
>  	u16 (*read_word)(struct mtd_info *mtd);
> +	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
>  	void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
>  	void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
>  	void (*select_chip)(struct mtd_info *mtd, int chip);
> -- 
> 1.8.2.rc2
> 
> 

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

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

[Huang Shijie]

于 2013年03月05日 00:50, Uwe Kleine-König 写道:
> @Huang Shijie: I didn't carry over your Ack to this new patch. Are you
> still happy with this one?
it's ok. :)


thanks
Huang Shijie

Re: [PATCH v2] mtdchar: handle chips that have user otp but no factory otp

[Artem Bityutskiy]

On Mon, 2013-03-04 at 17:35 +0100, Uwe Kleine-König wrote:
> Before this patch mtd_read_fact_prot_reg was used to check availability
> for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
> user otp for chips that don't have a factory otp area impossible. So use
> the right wrapper depending on the intended area to be accessed.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Forwarded: id:1361182768-31919-1-git-send-email-u.kleine-koenig@pengutronix.de

Pushed to l2-mtd.git, thanks.

I've removed the "Forwarded:" tag since I've never seen these to be used
before and not sure if this is an agreed practice.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2] mtdchar: handle chips that have user otp but no factory otp

[Uwe Kleine-König]

On Wed, Mar 06, 2013 at 10:47:05AM +0200, Artem Bityutskiy wrote:
> On Mon, 2013-03-04 at 17:35 +0100, Uwe Kleine-König wrote:
> > Before this patch mtd_read_fact_prot_reg was used to check availability
> > for both MTD_OTP_FACTORY and MTD_OTP_USER access. This made accessing
> > user otp for chips that don't have a factory otp area impossible. So use
> > the right wrapper depending on the intended area to be accessed.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Forwarded: id:1361182768-31919-1-git-send-email-u.kleine-koenig@pengutronix.de
> 
> Pushed to l2-mtd.git, thanks.
> 
> I've removed the "Forwarded:" tag since I've never seen these to be used
> before and not sure if this is an agreed practice.
We use this internally at Pengutronix to keep track of what is already
sent. Usually I remove the tag before resubmission.

Having said that I think it doesn't hurt and tip documents the message
id, too. But there are more urgent problems to solve and I'm fine with
dropping the tag.

Thanks
Uwe

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

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

[Artem Bityutskiy]

On Mon, 2013-03-04 at 17:47 +0100, Uwe Kleine-König wrote:
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 7ccb3c5..8ee4ed1 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -419,6 +419,8 @@ struct nand_buffers {
>   *                     flash device.
>   * @read_byte:         [REPLACEABLE] read one byte from the chip
>   * @read_word:         [REPLACEABLE] read one word from the chip
> + * @write_byte         [REPLACEABLE] write a single byte to the chip on the
> + *                     low 8 I/O lines
>   * @write_buf:         [REPLACEABLE] write data from the buffer to the chip
>   * @read_buf:          [REPLACEABLE] read data from the chip into the buffer
>   * @select_chip:       [REPLACEABLE] select chip nr
> @@ -503,6 +505,7 @@ struct nand_chip {
>  
>         uint8_t (*read_byte)(struct mtd_info *mtd);
>         u16 (*read_word)(struct mtd_info *mtd);
> +       void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
>         void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
>         void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
>         void (*select_chip)(struct mtd_info *mtd, int chip);

I would like to make the interface symmetric for read and write. I do
not have any 16 bit flashes, so need to somehow encouredge someone who
has one to do this. Namely, I'd like to kill 'read_word()', if possible.
What's your take on this?

-- 
Best Regards,
Artem Bityutskiy

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

[Artem Bityutskiy]

On Mon, 2013-03-04 at 17:47 +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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

On Mon, 2013-03-04 at 17:47 +0100, Uwe Kleine-König wrote:
> + */
> +static void nand_write_byte16(struct mtd_info *mtd, uint8_t byte)
> +{
> +       struct nand_chip *chip = mtd->priv;
> +
> +       /*
> +        * 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, (uint8_t[]){ byte, 0 }, 2);

Ick. Can you declare that array separately, rather than in the function
call please?

And also this seems broken on big-endian. nand_write_buf16() will cast
the 'buf' pointer to a uint16_t *, load a single uint16_t from it (for
example 0x5a00 if byte==0x5a), and proceed to write the 'byte' value on
the *high* side of the data bus, not the low side. Won't it?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[Uwe Kleine-König]

On Wed, Mar 13, 2013 at 11:33:04AM +0200, Artem Bityutskiy wrote:
> On Mon, 2013-03-04 at 17:47 +0100, Uwe Kleine-König wrote:
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 7ccb3c5..8ee4ed1 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -419,6 +419,8 @@ struct nand_buffers {
> >   *                     flash device.
> >   * @read_byte:         [REPLACEABLE] read one byte from the chip
> >   * @read_word:         [REPLACEABLE] read one word from the chip
> > + * @write_byte         [REPLACEABLE] write a single byte to the chip on the
> > + *                     low 8 I/O lines
> >   * @write_buf:         [REPLACEABLE] write data from the buffer to the chip
> >   * @read_buf:          [REPLACEABLE] read data from the chip into the buffer
> >   * @select_chip:       [REPLACEABLE] select chip nr
> > @@ -503,6 +505,7 @@ struct nand_chip {
> >  
> >         uint8_t (*read_byte)(struct mtd_info *mtd);
> >         u16 (*read_word)(struct mtd_info *mtd);
> > +       void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
> >         void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
> >         void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
> >         void (*select_chip)(struct mtd_info *mtd, int chip);
> 
> I would like to make the interface symmetric for read and write. I do
> not have any 16 bit flashes, so need to somehow encouredge someone who
> has one to do this. Namely, I'd like to kill 'read_word()', if possible.
> What's your take on this?
I don't have a 16 bit flash, so I have the same excuse as you do :-)

Uwe

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

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

[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.

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@pengutronix.de>
---
 drivers/mtd/nand/nand_base.c | 61 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/mtd/nand.h     |  3 +++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bd39f7b..27d755b 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);
+}
+
+/**
+ * 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);
+}
+
+/**
  * nand_write_buf - [DEFAULT] write buffer to chip
  * @mtd: MTD device structure
  * @buf: data buffer
@@ -2706,7 +2751,7 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
- * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
+ * nand_onfi_set_features - [REPLACEABLE] set features for ONFI nand
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
@@ -2716,6 +2761,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
 	int status;
+	int i;
 
 	if (!chip->onfi_version ||
 	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
@@ -2723,7 +2769,9 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 		return -EINVAL;
 
 	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
-	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		chip->write_byte(mtd, subfeature_param[i]);
+
 	status = chip->waitfunc(mtd, chip);
 	if (status & NAND_STATUS_FAIL)
 		return -EIO;
@@ -2731,7 +2779,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
- * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
+ * nand_onfi_get_features - [REPLACEABLE] get features for ONFI nand
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
@@ -2740,6 +2788,8 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
+	int i;
+
 	if (!chip->onfi_version ||
 	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
 	      & ONFI_OPT_CMD_SET_GET_FEATURES))
@@ -2749,7 +2799,8 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 	memset(subfeature_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
 
 	chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES, addr, -1);
-	chip->read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		*subfeature_param++ = chip->read_byte(mtd);
 	return 0;
 }
 
@@ -2812,6 +2863,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 		chip->block_markbad = nand_default_block_markbad;
 	if (!chip->write_buf || chip->write_buf == nand_write_buf)
 		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
+	if (!chip->write_byte)
+		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
 	if (!chip->read_buf || chip->read_buf == nand_read_buf)
 		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
 	if (!chip->scan_bbt)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9e6c8f9..e843942 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -432,6 +432,8 @@ struct nand_buffers {
  *			flash device.
  * @read_byte:		[REPLACEABLE] read one byte from the chip
  * @read_word:		[REPLACEABLE] read one word from the chip
+ * @write_byte		[REPLACEABLE] write a single byte to the chip on the
+ *			low 8 I/O lines
  * @write_buf:		[REPLACEABLE] write data from the buffer to the chip
  * @read_buf:		[REPLACEABLE] read data from the chip into the buffer
  * @select_chip:	[REPLACEABLE] select chip nr
@@ -521,6 +523,7 @@ struct nand_chip {
 
 	uint8_t (*read_byte)(struct mtd_info *mtd);
 	u16 (*read_word)(struct mtd_info *mtd);
+	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
 	void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
 	void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
 	void (*select_chip)(struct mtd_info *mtd, int chip);
-- 
1.8.4.2

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

[Brian Norris]

+ Pekon, Ezequiel

On Tue, Nov 26, 2013 at 10:03:41PM +0100, Uwe Kleine-König wrote:
> On Wed, Mar 13, 2013 at 11:33:04AM +0200, Artem Bityutskiy wrote:

Resurrecting a really old thread, eh? :) I like it!

> > On Mon, 2013-03-04 at 17:47 +0100, Uwe Kleine-König wrote:
> > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > > index 7ccb3c5..8ee4ed1 100644
> > > --- a/include/linux/mtd/nand.h
> > > +++ b/include/linux/mtd/nand.h
> > > @@ -419,6 +419,8 @@ struct nand_buffers {
> > >   *                     flash device.
> > >   * @read_byte:         [REPLACEABLE] read one byte from the chip
> > >   * @read_word:         [REPLACEABLE] read one word from the chip
> > > + * @write_byte         [REPLACEABLE] write a single byte to the chip on the
> > > + *                     low 8 I/O lines
> > >   * @write_buf:         [REPLACEABLE] write data from the buffer to the chip
> > >   * @read_buf:          [REPLACEABLE] read data from the chip into the buffer
> > >   * @select_chip:       [REPLACEABLE] select chip nr
> > > @@ -503,6 +505,7 @@ struct nand_chip {
> > >  
> > >         uint8_t (*read_byte)(struct mtd_info *mtd);
> > >         u16 (*read_word)(struct mtd_info *mtd);
> > > +       void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
> > >         void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
> > >         void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
> > >         void (*select_chip)(struct mtd_info *mtd, int chip);
> > 
> > I would like to make the interface symmetric for read and write. I do
> > not have any 16 bit flashes, so need to somehow encouredge someone who
> > has one to do this. Namely, I'd like to kill 'read_word()', if possible.
> > What's your take on this?
> I don't have a 16 bit flash, so I have the same excuse as you do :-)

Ezequiel and Pekon have 16-bit devices and can do testing, I think. They
also may be interested in your v3 patch.

Brian

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

[Brian Norris]

+ Pekon, Ezequiel

Can one of you see how this patch works with your BeagleBones w/ x16
NAND?

Also, do you think on of you could take a look at killing the
nand_chip.read_word() callback, as Artem was originally requesting with
this patch? It only has one user (for bad block checking in
nand_block_bad(), which is currently only used when there is no BBT at
all), and it looks like it could be replaced with a call to
read_buf(mtd, buf, 2).

Hi Uwe,

On Tue, Nov 26, 2013 at 10:15:15PM +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@pengutronix.de>
> ---
>  drivers/mtd/nand/nand_base.c | 61 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/mtd/nand.h     |  3 +++
>  2 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b..27d755b 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
...
> @@ -2706,7 +2751,7 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  }
>  
>  /**
> - * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
> + * nand_onfi_set_features - [REPLACEABLE] set features for ONFI nand

This change is unrelated. (Yes, the whitespace should be fixed, but
probably not in this patch.)

>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
...
> @@ -2731,7 +2779,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
>  }
>  
>  /**
> - * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
> + * nand_onfi_get_features - [REPLACEABLE] get features for ONFI nand

Ditto.

>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
...
> @@ -2812,6 +2863,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
>  		chip->block_markbad = nand_default_block_markbad;
>  	if (!chip->write_buf || chip->write_buf == nand_write_buf)
>  		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
> +	if (!chip->write_byte)

This check should be:

	if (!chip->write_byte || chip->write_byte == nand_write_byte)

in order to match the rest of the functions, so they can be reset if the
buswidth is detected to be x16, and we re-call nand_set_defaults()
(e.g., when using NAND_BUSWIDTH_AUTO). See the comment:

  /* If called twice, pointers that depend on busw may need to be reset */

I know it's kind of ugly, but that's what we have for now.

> +		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
>  	if (!chip->read_buf || chip->read_buf == nand_read_buf)
>  		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
>  	if (!chip->scan_bbt)

Brian

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

[Ezequiel Garcia]

Brian,

On Tue, Nov 26, 2013 at 11:35:12PM -0800, Brian Norris wrote:
> + Pekon, Ezequiel
> 
> Can one of you see how this patch works with your BeagleBones w/ x16
> NAND?
> 
> Also, do you think on of you could take a look at killing the
> nand_chip.read_word() callback, as Artem was originally requesting with
> this patch? It only has one user (for bad block checking in
> nand_block_bad(), which is currently only used when there is no BBT at
> all), and it looks like it could be replaced with a call to
> read_buf(mtd, buf, 2).
> 

Sure, sounds doable.

> Hi Uwe,
> 

Uwe,

If you're preparing a v4 to address Brian's feedback, then I'll see about testing
it with 8/16-bit flashes on my AM335x.

> On Tue, Nov 26, 2013 at 10:15:15PM +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@pengutronix.de>
> > ---
> >  drivers/mtd/nand/nand_base.c | 61 +++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/mtd/nand.h     |  3 +++
> >  2 files changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index bd39f7b..27d755b 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> ...
> > @@ -2706,7 +2751,7 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
> >  }
> >  
> >  /**
> > - * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
> > + * nand_onfi_set_features - [REPLACEABLE] set features for ONFI nand
> 
> This change is unrelated. (Yes, the whitespace should be fixed, but
> probably not in this patch.)
> 
> >   * @mtd: MTD device structure
> >   * @chip: nand chip info structure
> >   * @addr: feature address.
> ...
> > @@ -2731,7 +2779,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> >  }
> >  
> >  /**
> > - * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
> > + * nand_onfi_get_features - [REPLACEABLE] get features for ONFI nand
> 
> Ditto.
> 
> >   * @mtd: MTD device structure
> >   * @chip: nand chip info structure
> >   * @addr: feature address.
> ...
> > @@ -2812,6 +2863,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
> >  		chip->block_markbad = nand_default_block_markbad;
> >  	if (!chip->write_buf || chip->write_buf == nand_write_buf)
> >  		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
> > +	if (!chip->write_byte)
> 
> This check should be:
> 
> 	if (!chip->write_byte || chip->write_byte == nand_write_byte)
> 
> in order to match the rest of the functions, so they can be reset if the
> buswidth is detected to be x16, and we re-call nand_set_defaults()
> (e.g., when using NAND_BUSWIDTH_AUTO). See the comment:
> 
>   /* If called twice, pointers that depend on busw may need to be reset */
> 
> I know it's kind of ugly, but that's what we have for now.
> 
> > +		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
> >  	if (!chip->read_buf || chip->read_buf == nand_read_buf)
> >  		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
> >  	if (!chip->scan_bbt)
> 
> Brian

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

Hi Ezequiel,

On Fri, Nov 29, 2013 at 09:20:19AM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 26, 2013 at 11:35:12PM -0800, Brian Norris wrote:
> > + Pekon, Ezequiel
> > 
> > Can one of you see how this patch works with your BeagleBones w/ x16
> > NAND?

I see that you are pushing to straighten out the auto-buswidth part of
nand_base, and I think there may be good reasons to do so. But I think
that part of your problem can be resolved by a patch like Uwe's, where
rather than forcing the entire driver to be configured for x8 just to
use ONFI, we can fix the ONFI operations to use the lower 8 bits.

IOW, I expect that a patch like Uwe's can shed some better light on the
auto-buswidh situation. (This is why I CC'd you and Pekon.)

Unfortunately, I realized that Uwe's patch doesn't go far enough, I
don't think. It looks like it needs something like the following diff
(only compile-tested).

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bd39f7b67906..1ab264457d94 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 					int *busw)
 {
 	struct nand_onfi_params *p = &chip->onfi_params;
-	int i;
+	int i, j;
 	int val;
 
 	/* Try ONFI for unknown chip or LP */
@@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
 		return 0;
 
-	/*
-	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
-	 * with NAND_BUSWIDTH_16
-	 */
-	if (chip->options & NAND_BUSWIDTH_16) {
-		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
-		return 0;
-	}
-
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
-		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+		for (j = 0; j < sizeof(*p); j++)
+			*(uint8_t *)p = chip->read_byte(mtd);
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
 			break;

What do you think? (And more importantly, how does this test out for
you?)

Brian

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

[Ezequiel Garcia]

On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> Hi Ezequiel,
> 
> On Fri, Nov 29, 2013 at 09:20:19AM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 26, 2013 at 11:35:12PM -0800, Brian Norris wrote:
> > > + Pekon, Ezequiel
> > > 
> > > Can one of you see how this patch works with your BeagleBones w/ x16
> > > NAND?
> 
> I see that you are pushing to straighten out the auto-buswidth part of
> nand_base, and I think there may be good reasons to do so. But I think
> that part of your problem can be resolved by a patch like Uwe's, where
> rather than forcing the entire driver to be configured for x8 just to
> use ONFI, we can fix the ONFI operations to use the lower 8 bits.
> 
> IOW, I expect that a patch like Uwe's can shed some better light on the
> auto-buswidh situation. (This is why I CC'd you and Pekon.)
> 
> Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> don't think. It looks like it needs something like the following diff
> (only compile-tested).
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b67906..1ab264457d94 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  					int *busw)
>  {
>  	struct nand_onfi_params *p = &chip->onfi_params;
> -	int i;
> +	int i, j;
>  	int val;
>  
>  	/* Try ONFI for unknown chip or LP */
> @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
>  		return 0;
>  
> -	/*
> -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> -	 * with NAND_BUSWIDTH_16
> -	 */
> -	if (chip->options & NAND_BUSWIDTH_16) {
> -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> -		return 0;
> -	}
> -
>  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>  	for (i = 0; i < 3; i++) {
> -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> +		for (j = 0; j < sizeof(*p); j++)
> +			*(uint8_t *)p = chip->read_byte(mtd);
>  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>  				le16_to_cpu(p->crc)) {
>  			break;
> 
> What do you think? (And more importantly, how does this test out for
> you?)
> 

Let me try this out and let you know.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> 
> Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> don't think. It looks like it needs something like the following diff
> (only compile-tested).
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b67906..1ab264457d94 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  					int *busw)
>  {
>  	struct nand_onfi_params *p = &chip->onfi_params;
> -	int i;
> +	int i, j;
>  	int val;
>  
>  	/* Try ONFI for unknown chip or LP */
> @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
>  		return 0;
>  
> -	/*
> -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> -	 * with NAND_BUSWIDTH_16
> -	 */
> -	if (chip->options & NAND_BUSWIDTH_16) {
> -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> -		return 0;
> -	}
> -
>  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>  	for (i = 0; i < 3; i++) {
> -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> +		for (j = 0; j < sizeof(*p); j++)
> +			*(uint8_t *)p = chip->read_byte(mtd);
>  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>  				le16_to_cpu(p->crc)) {
>  			break;
> 
> What do you think? (And more importantly, how does this test out for
> you?)
> 

Your proposal would work (fixing a minor typo for incrementing 'p'),
except the nand_command() implementation messes with the buswith.
Therefore, after a long debugging session, I could make it work using
this hack:

@@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 	/* Serially input address */
 	if (column != -1) {
 		/* Adjust columns for 16 bit buswidth */
-		if (chip->options & NAND_BUSWIDTH_16)
-			column >>= 1;
+//		if (chip->options & NAND_BUSWIDTH_16)
+//			column >>= 1;
 		chip->cmd_ctrl(mtd, column, ctrl);
 		ctrl &= ~NAND_CTRL_CHANGE;
 	}

Now, IMHO, the solution of setting the defaults to x8 during the device
detection phase, is far simpler.

BTW: this is not really related to Uwe's patch, right? It's just a complement
to his work, as far as I can see.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote:
> On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> > 
> > Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> > don't think. It looks like it needs something like the following diff
> > (only compile-tested).
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index bd39f7b67906..1ab264457d94 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  					int *busw)
> >  {
> >  	struct nand_onfi_params *p = &chip->onfi_params;
> > -	int i;
> > +	int i, j;
> >  	int val;
> >  
> >  	/* Try ONFI for unknown chip or LP */
> > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> >  		return 0;
> >  
> > -	/*
> > -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> > -	 * with NAND_BUSWIDTH_16
> > -	 */
> > -	if (chip->options & NAND_BUSWIDTH_16) {
> > -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> > -		return 0;
> > -	}
> > -
> >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> >  	for (i = 0; i < 3; i++) {
> > -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > +		for (j = 0; j < sizeof(*p); j++)
> > +			*(uint8_t *)p = chip->read_byte(mtd);
> >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >  				le16_to_cpu(p->crc)) {
> >  			break;
> > 
> > What do you think? (And more importantly, how does this test out for
> > you?)
> > 
> 
> Your proposal would work (fixing a minor typo for incrementing 'p'),
> except the nand_command() implementation messes with the buswith.
> Therefore, after a long debugging session, I could make it work using
> this hack:
> 
> @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
>  	/* Serially input address */
>  	if (column != -1) {
>  		/* Adjust columns for 16 bit buswidth */
> -		if (chip->options & NAND_BUSWIDTH_16)
> -			column >>= 1;
> +//		if (chip->options & NAND_BUSWIDTH_16)
> +//			column >>= 1;
>  		chip->cmd_ctrl(mtd, column, ctrl);
>  		ctrl &= ~NAND_CTRL_CHANGE;
>  	}
> 
> Now, IMHO, the solution of setting the defaults to x8 during the device
> detection phase, is far simpler.
> 

And after some more debugging, I now realise the above problem is also
present at my previous proposal. So it seems we would need to actually
temporary "deactivate" NAND_BUSWIDTH_16 from chip->options.

I wonder how ugly that could be. Comments?

In any case, it seems our proposals are equivalent:
* we can change the defaults to x8 (is this at all needed?)
* we can use read_byte

But, again, we'll need to unset NAND_BUSWIDTH_16 from chip->option.

Now, it would be a bit confusing to "trick" NAND_BUSWIDTH_16 from
options without also setting the defaults to x8 for the detection phase.

Comments?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

On Sat, Nov 30, 2013 at 01:35:36PM -0300, Ezequiel Garcia wrote:
> On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> > 
> > Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> > don't think. It looks like it needs something like the following diff
> > (only compile-tested).
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index bd39f7b67906..1ab264457d94 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  					int *busw)
> >  {
> >  	struct nand_onfi_params *p = &chip->onfi_params;
> > -	int i;
> > +	int i, j;
> >  	int val;
> >  
> >  	/* Try ONFI for unknown chip or LP */
> > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> >  		return 0;
> >  
> > -	/*
> > -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> > -	 * with NAND_BUSWIDTH_16
> > -	 */
> > -	if (chip->options & NAND_BUSWIDTH_16) {
> > -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> > -		return 0;
> > -	}
> > -
> >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> >  	for (i = 0; i < 3; i++) {
> > -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > +		for (j = 0; j < sizeof(*p); j++)
> > +			*(uint8_t *)p = chip->read_byte(mtd);
> >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >  				le16_to_cpu(p->crc)) {
> >  			break;
> > 
> > What do you think? (And more importantly, how does this test out for
> > you?)
> > 
> 
> Your proposal would work (fixing a minor typo for incrementing 'p'),
> except the nand_command() implementation messes with the buswith.
> Therefore, after a long debugging session, I could make it work using
> this hack:
> 
> @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
>  	/* Serially input address */
>  	if (column != -1) {
>  		/* Adjust columns for 16 bit buswidth */
> -		if (chip->options & NAND_BUSWIDTH_16)
> -			column >>= 1;
> +//		if (chip->options & NAND_BUSWIDTH_16)
> +//			column >>= 1;
>  		chip->cmd_ctrl(mtd, column, ctrl);
>  		ctrl &= ~NAND_CTRL_CHANGE;
>  	}

Hmm, that does seem like a hack. What command is giving you problems, by
the way? And is the NAND operational after this hack?

It looks like those two lines are there so that we can always specify
'column' in bytes, and nand_command() will do the byte/word translation
automatically. But we don't want this for certain commands, I think, so
maybe a "hack" is necessary...

> Now, IMHO, the solution of setting the defaults to x8 during the device
> detection phase, is far simpler.
> 
> BTW: this is not really related to Uwe's patch, right? It's just a complement
> to his work, as far as I can see.

Yeah, it's a complement. At first, I though Uwe's patch included this,
but I was wrong.

Brian

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

[Brian Norris]

On Sat, Nov 30, 2013 at 01:51:23PM -0300, Ezequiel Garcia wrote:
> On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote:
> > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> > > 
> > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> > > don't think. It looks like it needs something like the following diff
> > > (only compile-tested).
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index bd39f7b67906..1ab264457d94 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  					int *busw)
> > >  {
> > >  	struct nand_onfi_params *p = &chip->onfi_params;
> > > -	int i;
> > > +	int i, j;
> > >  	int val;
> > >  
> > >  	/* Try ONFI for unknown chip or LP */
> > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> > >  		return 0;
> > >  
> > > -	/*
> > > -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> > > -	 * with NAND_BUSWIDTH_16
> > > -	 */
> > > -	if (chip->options & NAND_BUSWIDTH_16) {
> > > -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> > > -		return 0;
> > > -	}
> > > -
> > >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > >  	for (i = 0; i < 3; i++) {
> > > -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > > +		for (j = 0; j < sizeof(*p); j++)
> > > +			*(uint8_t *)p = chip->read_byte(mtd);
> > >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > >  				le16_to_cpu(p->crc)) {
> > >  			break;
> > > 
> > > What do you think? (And more importantly, how does this test out for
> > > you?)
> > > 
> > 
> > Your proposal would work (fixing a minor typo for incrementing 'p'),
> > except the nand_command() implementation messes with the buswith.
> > Therefore, after a long debugging session, I could make it work using
> > this hack:
> > 
> > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
> >  	/* Serially input address */
> >  	if (column != -1) {
> >  		/* Adjust columns for 16 bit buswidth */
> > -		if (chip->options & NAND_BUSWIDTH_16)
> > -			column >>= 1;
> > +//		if (chip->options & NAND_BUSWIDTH_16)
> > +//			column >>= 1;
> >  		chip->cmd_ctrl(mtd, column, ctrl);
> >  		ctrl &= ~NAND_CTRL_CHANGE;
> >  	}
> > 
> > Now, IMHO, the solution of setting the defaults to x8 during the device
> > detection phase, is far simpler.
> > 
> 
> And after some more debugging, I now realise the above problem is also
> present at my previous proposal. So it seems we would need to actually
> temporary "deactivate" NAND_BUSWIDTH_16 from chip->options.
> 
> I wonder how ugly that could be. Comments?

I'm not sure yet. I'd like to better understand what command is failing,
and why.

> In any case, it seems our proposals are equivalent:
> * we can change the defaults to x8 (is this at all needed?)
> * we can use read_byte

No, our proposals are not equivalent.

Your patches are only solving these ONFI bus-width problems during
initialization. I believe we will want to use some ONFI routines (SET
FEATURES, especially) after initialization. This is where the rest of
Uwe's patch comes into play. So I don't think we can always switch
between call-backs and play games with NAND_BUSWIDTH_16; we should get
the bus width right as soon as possible, and then make sure that the
callbacks always work as expected.

You are also placing more burden on drivers. You require the drivers to
add failure logic if the NAND core auto-configures a buswidth that the
host doesn't support. I prefer that for cases where the bus width is
known a-priori, the driver only needs to call nand_scan(), and the NAND
core can error out appropriately.

> But, again, we'll need to unset NAND_BUSWIDTH_16 from chip->option.
> 
> Now, it would be a bit confusing to "trick" NAND_BUSWIDTH_16 from
> options without also setting the defaults to x8 for the detection phase.
> 
> Comments?

Brian

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

[Ezequiel Garcia]

On Sat, Nov 30, 2013 at 10:53:08AM -0800, Brian Norris wrote:
> On Sat, Nov 30, 2013 at 01:35:36PM -0300, Ezequiel Garcia wrote:
> > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> > > 
> > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> > > don't think. It looks like it needs something like the following diff
> > > (only compile-tested).
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index bd39f7b67906..1ab264457d94 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  					int *busw)
> > >  {
> > >  	struct nand_onfi_params *p = &chip->onfi_params;
> > > -	int i;
> > > +	int i, j;
> > >  	int val;
> > >  
> > >  	/* Try ONFI for unknown chip or LP */
> > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> > >  		return 0;
> > >  
> > > -	/*
> > > -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> > > -	 * with NAND_BUSWIDTH_16
> > > -	 */
> > > -	if (chip->options & NAND_BUSWIDTH_16) {
> > > -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> > > -		return 0;
> > > -	}
> > > -
> > >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > >  	for (i = 0; i < 3; i++) {
> > > -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > > +		for (j = 0; j < sizeof(*p); j++)
> > > +			*(uint8_t *)p = chip->read_byte(mtd);
> > >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > >  				le16_to_cpu(p->crc)) {
> > >  			break;
> > > 
> > > What do you think? (And more importantly, how does this test out for
> > > you?)
> > > 
> > 
> > Your proposal would work (fixing a minor typo for incrementing 'p'),
> > except the nand_command() implementation messes with the buswith.
> > Therefore, after a long debugging session, I could make it work using
> > this hack:
> > 
> > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
> >  	/* Serially input address */
> >  	if (column != -1) {
> >  		/* Adjust columns for 16 bit buswidth */
> > -		if (chip->options & NAND_BUSWIDTH_16)
> > -			column >>= 1;
> > +//		if (chip->options & NAND_BUSWIDTH_16)
> > +//			column >>= 1;
> >  		chip->cmd_ctrl(mtd, column, ctrl);
> >  		ctrl &= ~NAND_CTRL_CHANGE;
> >  	}
> 
> Hmm, that does seem like a hack. What command is giving you problems, by
> the way?

READ_ID

> And is the NAND operational after this hack?
> 

Yup, I guess so. It takes some time to prepare the hardware to test
that, so I'll be to re-test it properly to provide some more complete
answer.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On Sat, Nov 30, 2013 at 11:01:49AM -0800, Brian Norris wrote:
> On Sat, Nov 30, 2013 at 01:51:23PM -0300, Ezequiel Garcia wrote:
> > On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote:
> > > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote:
> > > > 
> > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I
> > > > don't think. It looks like it needs something like the following diff
> > > > (only compile-tested).
> > > > 
> > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > > index bd39f7b67906..1ab264457d94 100644
> > > > --- a/drivers/mtd/nand/nand_base.c
> > > > +++ b/drivers/mtd/nand/nand_base.c
> > > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > > >  					int *busw)
> > > >  {
> > > >  	struct nand_onfi_params *p = &chip->onfi_params;
> > > > -	int i;
> > > > +	int i, j;
> > > >  	int val;
> > > >  
> > > >  	/* Try ONFI for unknown chip or LP */
> > > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > > >  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> > > >  		return 0;
> > > >  
> > > > -	/*
> > > > -	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> > > > -	 * with NAND_BUSWIDTH_16
> > > > -	 */
> > > > -	if (chip->options & NAND_BUSWIDTH_16) {
> > > > -		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> > > > -		return 0;
> > > > -	}
> > > > -
> > > >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > > >  	for (i = 0; i < 3; i++) {
> > > > -		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > > > +		for (j = 0; j < sizeof(*p); j++)
> > > > +			*(uint8_t *)p = chip->read_byte(mtd);
> > > >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > > >  				le16_to_cpu(p->crc)) {
> > > >  			break;
> > > > 
> > > > What do you think? (And more importantly, how does this test out for
> > > > you?)
> > > > 
> > > 
> > > Your proposal would work (fixing a minor typo for incrementing 'p'),
> > > except the nand_command() implementation messes with the buswith.
> > > Therefore, after a long debugging session, I could make it work using
> > > this hack:
> > > 
> > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
> > >  	/* Serially input address */
> > >  	if (column != -1) {
> > >  		/* Adjust columns for 16 bit buswidth */
> > > -		if (chip->options & NAND_BUSWIDTH_16)
> > > -			column >>= 1;
> > > +//		if (chip->options & NAND_BUSWIDTH_16)
> > > +//			column >>= 1;
> > >  		chip->cmd_ctrl(mtd, column, ctrl);
> > >  		ctrl &= ~NAND_CTRL_CHANGE;
> > >  	}
> > > 
> > > Now, IMHO, the solution of setting the defaults to x8 during the device
> > > detection phase, is far simpler.
> > > 
> > 
> > And after some more debugging, I now realise the above problem is also
> > present at my previous proposal. So it seems we would need to actually
> > temporary "deactivate" NAND_BUSWIDTH_16 from chip->options.
> > 
> > I wonder how ugly that could be. Comments?
> 
> I'm not sure yet. I'd like to better understand what command is failing,
> and why.
> 
> > In any case, it seems our proposals are equivalent:
> > * we can change the defaults to x8 (is this at all needed?)
> > * we can use read_byte
> 
> No, our proposals are not equivalent.
> 
> Your patches are only solving these ONFI bus-width problems during
> initialization.

Agreed.

> I believe we will want to use some ONFI routines (SET
> FEATURES, especially) after initialization. This is where the rest of
> Uwe's patch comes into play. So I don't think we can always switch
> between call-backs and play games with NAND_BUSWIDTH_16; we should get
> the bus width right as soon as possible, and then make sure that the
> callbacks always work as expected.
> 

Sure, I completely understand the above. The patches I've been pushing
are meant *only* to solve the initial device detection, and in
particular to fix the currently broken ONFI detection.

I realise that Uwe's patches (and from what I've been seeing some more)
are needed in other to solve other ONFI width-related problems.

Just to clarify, the v2 I just sent was motivated by this rationale:
if we need to temporarily switch off NAND_BUSWIDTH_16, then it makes
sense to also switch the entire default functions.

However, if you can find a cleaner (i.e. ot too hacky) solution,
so we can prevent this width switching, I'd be happy to test it!

> You are also placing more burden on drivers. You require the drivers to
> add failure logic if the NAND core auto-configures a buswidth that the
> host doesn't support. I prefer that for cases where the bus width is
> known a-priori, the driver only needs to call nand_scan(), and the NAND
> core can error out appropriately.
> 

Yup, that's correct. Under my solution driver's _must_ handle a two phase
initialization: nand_scan_ident() + nand_scan_tail(); but only if they
need some special tweaking after the bus width discovering.

I admit my proposal might be narrow-minded, and biased by the only few
NAND drivers I'm familiar to :(
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Gupta, Pekon]

>From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
>> > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
>> > >  	/* Serially input address */
>> > >  	if (column != -1) {
>> > >  		/* Adjust columns for 16 bit buswidth */
>> > > -		if (chip->options & NAND_BUSWIDTH_16)
>> > > -			column >>= 1;
>> > > +//		if (chip->options & NAND_BUSWIDTH_16)
>> > > +//			column >>= 1;
>> > >  		chip->cmd_ctrl(mtd, column, ctrl);
>> > >  		ctrl &= ~NAND_CTRL_CHANGE;
>> > >  	}
>> > >
This is a very good finding ..
The ONFI spec says that data would be present only on lower 8-bits of
NAND data-bus. This mean we need to always issue 1-byte aligned address.
The above fix enforces that rule.

In absence of this fix, if (chip->options & NAND_BUSWIDTH_16) is set,
then nand_command() would issue a word-aligned address even when
we use chip->read_byte() to read a single data.

To take do this cleanly , we might need a larger fix, that is change in
interface function chip->cmdfunc()..
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
- void (*cmdfunc)(struct mtd_info *mtd, unsigned command, int column,
-			int page_addr);
+ void (*cmdfunc)(struct mtd_info *mtd, unsigned command, int column,
+			int page_addr, int busw);


And replace above hack with.. @@nand_command()
-	if (chip->options & NAND_BUSWIDTH_16)
-		column >>= 1;
+	if (busw)
+		column >>= 1;

[...]

>> I believe we will want to use some ONFI routines (SET
>> FEATURES, especially) after initialization. This is where the rest of
>> Uwe's patch comes into play. So I don't think we can always switch
>> between call-backs and play games with NAND_BUSWIDTH_16; we should get
>> the bus width right as soon as possible, and then make sure that the
>> callbacks always work as expected.
>>
>
>Sure, I completely understand the above. The patches I've been pushing
>are meant *only* to solve the initial device detection, and in
>particular to fix the currently broken ONFI detection.
>
So as per my understanding..
*Uwe's patch*
- handles byte-wise access for onfi_set_feature()/onfi_get_feature()
*Brian's patch*
 - handles byte-wise access for nand_flash_detect_onfi()
*Ezequiel's patch*
 - modifies nand_command() to issue byte-aligned accesses. (as above)
 - re-configuring of chip->options in nand_scan_ident before returning
    back to callee (controller driver here).

If correct, then we should group these pieces together in patch-series
(with all contributors signed-off). And then test this on multiple devices.

I can do that, if all agree that above pieces are correct and required for
completion of functionality ??


with regards, pekon

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

[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.

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@pengutronix.de>
---
Changes since v2, sent with
Message-Id: 1362415655-13073-1-git-send-email-u.kleine-koenig@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@pengutronix.de (v3)

 - drop whitespace cleanup
 - support resetting to 16 bit width function on 2nd call of nand_set_defaults

 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);
+}
+
+/**
+ * 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);
+}
+
+/**
  * nand_write_buf - [DEFAULT] write buffer to chip
  * @mtd: MTD device structure
  * @buf: data buffer
@@ -2716,6 +2761,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
 	int status;
+	int i;
 
 	if (!chip->onfi_version ||
 	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
@@ -2723,7 +2769,9 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 		return -EINVAL;
 
 	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
-	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		chip->write_byte(mtd, subfeature_param[i]);
+
 	status = chip->waitfunc(mtd, chip);
 	if (status & NAND_STATUS_FAIL)
 		return -EIO;
@@ -2740,6 +2788,8 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 			int addr, uint8_t *subfeature_param)
 {
+	int i;
+
 	if (!chip->onfi_version ||
 	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
 	      & ONFI_OPT_CMD_SET_GET_FEATURES))
@@ -2749,7 +2799,8 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 	memset(subfeature_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
 
 	chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES, addr, -1);
-	chip->read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		*subfeature_param++ = chip->read_byte(mtd);
 	return 0;
 }
 
@@ -2812,6 +2863,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 		chip->block_markbad = nand_default_block_markbad;
 	if (!chip->write_buf || chip->write_buf == nand_write_buf)
 		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
+	if (!chip->write_byte || chip->write_byte == nand_write_byte)
+		chip->write_byte = busw ? nand_write_byte16 : nand_write_byte;
 	if (!chip->read_buf || chip->read_buf == nand_read_buf)
 		chip->read_buf = busw ? nand_read_buf16 : nand_read_buf;
 	if (!chip->scan_bbt)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9e6c8f9..e843942 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -432,6 +432,8 @@ struct nand_buffers {
  *			flash device.
  * @read_byte:		[REPLACEABLE] read one byte from the chip
  * @read_word:		[REPLACEABLE] read one word from the chip
+ * @write_byte		[REPLACEABLE] write a single byte to the chip on the
+ *			low 8 I/O lines
  * @write_buf:		[REPLACEABLE] write data from the buffer to the chip
  * @read_buf:		[REPLACEABLE] read data from the chip into the buffer
  * @select_chip:	[REPLACEABLE] select chip nr
@@ -521,6 +523,7 @@ struct nand_chip {
 
 	uint8_t (*read_byte)(struct mtd_info *mtd);
 	u16 (*read_word)(struct mtd_info *mtd);
+	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
 	void (*write_buf)(struct mtd_info *mtd, const uint8_t *buf, int len);
 	void (*read_buf)(struct mtd_info *mtd, uint8_t *buf, int len);
 	void (*select_chip)(struct mtd_info *mtd, int chip);
-- 
1.8.4.3

Re: [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@pengutronix.de>
> ---
> Changes since v2, sent with
> Message-Id: 1362415655-13073-1-git-send-email-u.kleine-koenig@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@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

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

[Ezequiel Garcia]

On Mon, Dec 16, 2013 at 09:48:25PM -0800, Brian Norris wrote:
[..]
> 
> 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.
> 

No, I couldn't do a proper test of this one, although I remembered a
tested a branch based on this patch.

The problem was I couldn't find time to figure out how to test the GetFeatures
and SetFeatures command. Hints?

I can try to give it a test this weekend.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

On Tue, Dec 17, 2013 at 06:46:49PM -0300, Ezequiel Garcia wrote:
> The problem was I couldn't find time to figure out how to test the GetFeatures
> and SetFeatures command. Hints?

Browse the ONFI spec for something you should expect to be able to read
(for GET_FEATURES). Incidentally, I've been testing {GET,SET}_FEATURES
with my Micron MLC that need read-retry, but you might not have any
flash that support that.

Brian

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

[Brian Norris]

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@pengutronix.de>

I think this patch is good now, so pushed to l2-mtd.git (with a fix
[1]). Thanks Uwe!

I know there was some more discussion and other patch ideas being thrown
around (by me and others). Feel free to send patches/comments (or
Ack's/Tested-by's) if you think they need applied on top.

Brian

[1] I got a type safety warning as I pushed this out, so I squashed in
    this diff:

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7acbce3b3675..d388c7f6fec9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -245,7 +245,7 @@ static void nand_write_byte16(struct mtd_info *mtd, uint8_t byte)
 	 * 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);
+	chip->write_buf(mtd, (uint8_t *)&word, 2);
 }
 
 /**

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

[Uwe Kleine-König]

Hello,

On Tue, Jan 14, 2014 at 12:12:16AM -0800, Brian Norris wrote:
> 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@pengutronix.de>
> 
> I think this patch is good now, so pushed to l2-mtd.git (with a fix
> [1]). Thanks Uwe!
> 
> I know there was some more discussion and other patch ideas being thrown
> around (by me and others). Feel free to send patches/comments (or
> Ack's/Tested-by's) if you think they need applied on top.
> 
> Brian
> 
> [1] I got a type safety warning as I pushed this out, so I squashed in
>     this diff:
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 7acbce3b3675..d388c7f6fec9 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -245,7 +245,7 @@ static void nand_write_byte16(struct mtd_info *mtd, uint8_t byte)
>  	 * 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);
> +	chip->write_buf(mtd, (uint8_t *)&word, 2);
That's exactly what I suggested before reading your mail. Very
appreciated.

Uwe

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