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

[PATCH] 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 error out as there is no
write_byte callback.

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

note this is only compile tested and I don't have a 16-bit wide nand, so I
don't even saw a failure.

The problem exists since commit

	7db03ec (mtd: add helpers to set/get features for ONFI nand)

which introduced the two functions.

Still I'd like to know how I can write a byte (or a sequence of bytes) as this
is necessary for locking the otp are of micron chips. Well, I could implement
it for 8-bit chips only, but this isn't very satisfying.

Do we need to add a write_byte callback to struct nand_chip? Or is there
a way to do write a byte that I'm just too blind to see?

Is this patch relevant for stable? Probably not!?

Best regards
Uwe

 drivers/mtd/nand/nand_base.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4321415..abfd8ca 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2706,7 +2706,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,6 +2720,11 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 	if (!chip->onfi_version)
 		return -EINVAL;
 
+	if (chip->options & NAND_BUSWIDTH_16) {
+		pr_warn("onfi set feature command buggy for 16-bit chips\n");
+		return -ENOTSUPP;
+	}
+
 	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
 	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
 	status = chip->waitfunc(mtd, chip);
@@ -2729,7 +2734,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.
@@ -2738,6 +2743,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;
 
@@ -2745,7 +2752,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;
 }
 
-- 
1.7.10.4

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

[Huang Shijie]

于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> According to the Open NAND Flash Interface Specification (ONFI) Revision
> 3.1 "Parameters are always transferred on the lower 8-bits of the data
> bus." for the Get Features and Set Features commands.
>
yes. the set/get features should works in 8-bit.

I have never met a 16-bit onfi nand yet. :)

> So using read_buf and write_buf is wrong for 16-bit wide nand chips as
> they use I/O[15:0]. The Get Features command is easily fixed using 4
> times the read_byte callback. For Set Features error out as there is no
yes. for get features, it's easy to fix it.
> write_byte callback.
Most of the time, the nand controller will overwrite the write_buf hook...
I also think we need a write_byte callback.

thanks
Huang Shijie

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

[Uwe Kleine-König]

Hello,

On Thu, Feb 28, 2013 at 10:47:43AM +0800, Huang Shijie wrote:
> 于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> >According to the Open NAND Flash Interface Specification (ONFI) Revision
> >3.1 "Parameters are always transferred on the lower 8-bits of the data
> >bus." for the Get Features and Set Features commands.
> >
> yes. the set/get features should works in 8-bit.
> 
> I have never met a 16-bit onfi nand yet. :)
> 
> >So using read_buf and write_buf is wrong for 16-bit wide nand chips as
> >they use I/O[15:0]. The Get Features command is easily fixed using 4
> >times the read_byte callback. For Set Features error out as there is no
> yes. for get features, it's easy to fix it.
> >write_byte callback.
> Most of the time, the nand controller will overwrite the write_buf hook...
> I also think we need a write_byte callback.
Is this an Ack for my patch?

Best regards
Uwe

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

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

[Uwe Kleine-König]

On Thu, Feb 28, 2013 at 10:47:43AM +0800, Huang Shijie wrote:
> 于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> >According to the Open NAND Flash Interface Specification (ONFI) Revision
> >3.1 "Parameters are always transferred on the lower 8-bits of the data
> >bus." for the Get Features and Set Features commands.
> >
> yes. the set/get features should works in 8-bit.
> 
> I have never met a 16-bit onfi nand yet. :)
> 
> >So using read_buf and write_buf is wrong for 16-bit wide nand chips as
> >they use I/O[15:0]. The Get Features command is easily fixed using 4
> >times the read_byte callback. For Set Features error out as there is no
> yes. for get features, it's easy to fix it.
> >write_byte callback.
> Most of the time, the nand controller will overwrite the write_buf hook...
> I also think we need a write_byte callback.
a default implementation could be something like that:

	static void nand_write_byte(struct mtd_info *mtd, uint8_t byte)
	{
		struct nand_chip *chip = mtd->priv;

		if (chip->options & NAND_BUSWIDTH_16)
			chip->write_buf(mtd, (uint8_t[]){ byte, 0 }, 2);
		else
			chip->write_buf(mtd, &byte, 1);
	}

(Is this the correct order in the array? Or might that depend on
endianess?)

Does this look right?

Alternatively something could be done with chip->cmd_ctrl (but it seems
not all drivers implement this, e.g. mxc_nand doesn't).

Best regards
Uwe

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

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

[Huang Shijie]

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

In my opinion, the default nand_write_byte() hook could use the 
nand_write_buf() to  write just one byte;
and the nand controller can overwrite the nand_write_byte() hook if it 
could.
Of course, it's just a suggest.

thanks
Huang Shijie



> Alternatively something could be done with chip->cmd_ctrl (but it seems
> not all drivers implement this, e.g. mxc_nand doesn't).
>
> Best regards
> Uwe
>

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

[Uwe Kleine-König]

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

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

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

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

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

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

Best regards
Uwe

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

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

[Huang Shijie]

于 2013年03月01日 16:50, Uwe Kleine-König 写道:
> puts $byte to I/O[7:0] and 0 to I/O[15:8]. This is what I want it to
> do---not sure if I/O[15:8] should better be tri-stated?
I am not sure too. :)
I do not ever use a 16-bit nand controller.

thanks
Huang Shijie

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

[Matthieu CASTET]

Huang Shijie a écrit :
> 于 2013年02月27日 23:10, Uwe Kleine-König 写道:
>> According to the Open NAND Flash Interface Specification (ONFI) Revision
>> 3.1 "Parameters are always transferred on the lower 8-bits of the data
>> bus." for the Get Features and Set Features commands.
>>
> yes. the set/get features should works in 8-bit.
> 
> I have never met a 16-bit onfi nand yet. :)

Beagle board got one.


Matthieu

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

[Uwe Kleine-König]

On Fri, Mar 01, 2013 at 10:20:53AM +0100, Matthieu CASTET wrote:
> Huang Shijie a écrit :
> > 于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> >> According to the Open NAND Flash Interface Specification (ONFI) Revision
> >> 3.1 "Parameters are always transferred on the lower 8-bits of the data
> >> bus." for the Get Features and Set Features commands.
> >>
> > yes. the set/get features should works in 8-bit.
> > 
> > I have never met a 16-bit onfi nand yet. :)
> 
> Beagle board got one.
Which part does it have? Would you volunteer to test access to its OTP
area? (Provided you have a Micron chip of course.)

Best regards
Uwe

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

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

[Matthieu CASTET]

Uwe Kleine-König a écrit :
> On Fri, Mar 01, 2013 at 10:20:53AM +0100, Matthieu CASTET wrote:
>> Huang Shijie a écrit :
>>> 于 2013年02月27日 23:10, Uwe Kleine-König 写道:
>>>> According to the Open NAND Flash Interface Specification (ONFI) Revision
>>>> 3.1 "Parameters are always transferred on the lower 8-bits of the data
>>>> bus." for the Get Features and Set Features commands.
>>>>
>>> yes. the set/get features should works in 8-bit.
>>>
>>> I have never met a 16-bit onfi nand yet. :)
>> Beagle board got one.
> Which part does it have? Would you volunteer to test access to its OTP
> area? (Provided you have a Micron chip of course.)
> 
It is a micron chip (MT29F2G16ABD), but I can't test on it ATM.


Matthieu

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

[Huang Shijie]

于 2013年02月27日 23:10, Uwe Kleine-König 写道:
> According to the Open NAND Flash Interface Specification (ONFI) Revision
> 3.1 "Parameters are always transferred on the lower 8-bits of the data
> bus." for the Get Features and Set Features commands.
>
> So using read_buf and write_buf is wrong for 16-bit wide nand chips as
> they use I/O[15:0]. The Get Features command is easily fixed using 4
> times the read_byte callback. For Set Features error out as there is no
> write_byte callback.
>
> Signed-off-by: Uwe Kleine-König<u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> note this is only compile tested and I don't have a 16-bit wide nand, so I
> don't even saw a failure.
>
> The problem exists since commit
>
> 	7db03ec (mtd: add helpers to set/get features for ONFI nand)
>
> which introduced the two functions.
>
> Still I'd like to know how I can write a byte (or a sequence of bytes) as this
> is necessary for locking the otp are of micron chips. Well, I could implement
> it for 8-bit chips only, but this isn't very satisfying.
>
> Do we need to add a write_byte callback to struct nand_chip? Or is there
> a way to do write a byte that I'm just too blind to see?
>
> Is this patch relevant for stable? Probably not!?
>
> Best regards
> Uwe
>
>   drivers/mtd/nand/nand_base.c |   14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 4321415..abfd8ca 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2706,7 +2706,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,6 +2720,11 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
>   	if (!chip->onfi_version)
>   		return -EINVAL;
>
> +	if (chip->options&  NAND_BUSWIDTH_16) {
> +		pr_warn("onfi set feature command buggy for 16-bit chips\n");
> +		return -ENOTSUPP;
> +	}
> +
>   	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
>   	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
>   	status = chip->waitfunc(mtd, chip);
> @@ -2729,7 +2734,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.
> @@ -2738,6 +2743,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;
>
> @@ -2745,7 +2752,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;
>   }
>
Acked-by: Huang Shijie <b32955@freescale.com>