| Message ID | 1361977852-18233-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
于 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
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
于 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>
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
于 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 >
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
于 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
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
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
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
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; }
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(-)