| Message ID | 1489513548-3248-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Wed, 15 Mar 2017 02:45:48 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > The nand_default_block_markbad() is the default implementation of > chip->block_markbad(). This is called for marking a block as bad. > It invokes nand_do_write_oob(), then calls a higher level accessor > ecc->write_oob(). > > On the other hand, when reading BBM from the OOB, chip->block_bad() > is called, and nand_block_bad() is the default implementation. This > function calls a lower level chip->cmdfunc(). If a driver wants to > re-use nand_block_bad(), it is required to support NAND_CMD_READOOB > in its cmdfunc(). This is part of the basic/mandatory operations that should be supported by all drivers. > This is strange. If the controller supports > optimized read operation and the driver has its own ecc->read_oob(), > it should be able to use it. I agree with this one. I guess the idea behind this default implementation was to avoid reading the whole OOB area, or maybe this function was implemented before we had ECC support. Anyway, the overhead should be negligible with your approach. > Besides, NAND_CMD_READOOB (0x50) is > not a real command for large page devices. So, recent drivers may > not be happy to handle this command. Well, that's the whole problem with the ->cmdfunc() hook, even if it's passed raw NAND command identifiers, these are actually encoding NAND operations, and not necessarily the exact command that should be sent to the NAND. See what's done in nand_command_lp(), and how some commands are actually generating a sequence of 2 commands [1], or how NAND_CMD_READOOB is transformed into NAND_CMD_READ0 [2]. My plan was to add a ->exec_op() hook that would be passed the whole operation description (cmds+addrs+data info) and progressively get rid of ->cmdfunc(), but I never had time to finish the implementation[3]. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > At first, I tried to call nand_do_read_oob() from nand_block_bad() > in order to make to make things symmetry, but it did not work. > chip->select_chip() is handled outside of nand_block_bad(), so > nand_do_read_oob() can not be used there. > > > drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++--------------------- > 1 file changed, 17 insertions(+), 21 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index a3c0f47..78c5cd6 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -354,9 +354,9 @@ static void nand_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len) > */ > static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) > { > - int page, res = 0, i = 0; > + int page, res = 0, ret = 0, i = 0; > struct nand_chip *chip = mtd_to_nand(mtd); > - u16 bad; > + u8 bad; > > if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) > ofs += mtd->erasesize - mtd->writesize; > @@ -364,30 +364,26 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) > page = (int)(ofs >> chip->page_shift) & chip->pagemask; > > do { > - if (chip->options & NAND_BUSWIDTH_16) { > - chip->cmdfunc(mtd, NAND_CMD_READOOB, > - chip->badblockpos & 0xFE, page); > - bad = cpu_to_le16(chip->read_word(mtd)); > - if (chip->badblockpos & 0x1) > - bad >>= 8; > + res = chip->ecc.read_oob(mtd, chip, page); > + if (!res) { > + bad = chip->oob_poi[chip->badblockpos]; Hm, even if the current code is only testing one byte, I wonder if we shouldn't test the 2 bytes here when we're dealing with 16bits NANDs. > + > + if (likely(chip->badblockbits == 8)) > + res = bad != 0xFF; > else > - bad &= 0xFF; > - } else { > - chip->cmdfunc(mtd, NAND_CMD_READOOB, chip->badblockpos, > - page); > - bad = chip->read_byte(mtd); > + res = hweight8(bad) < chip->badblockbits; > + if (res) > + return res; > } > > - if (likely(chip->badblockbits == 8)) > - res = bad != 0xFF; > - else > - res = hweight8(bad) < chip->badblockbits; > - ofs += mtd->writesize; > - page = (int)(ofs >> chip->page_shift) & chip->pagemask; > + if (!ret) > + ret = res; Hm, I'm not sure I understand what you're doing here. If res is != 0, then an error occurred when reading the OOB area and you should probably return the error code directly instead of trying to read the OOB from next page. On the other hand, if res is 0, then ret will always be 0, so I don't think this extra ret variable is needed. > + > + page++; > i++; > - } while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)); > + } while (chip->bbt_options & NAND_BBT_SCAN2NDPAGE && i < 2); A for loop would probably make more sense here, but that's just a detail. > > - return res; > + return ret; > } > > /** [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L824 [2]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L747 [3]https://github.com/bbrezillon/linux-sunxi/blob/nand-core-rework-v2/include/linux/mtd/nand2.h
Hi Boris, Thanks for your review. 2017-03-15 5:58 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Wed, 15 Mar 2017 02:45:48 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> The nand_default_block_markbad() is the default implementation of >> chip->block_markbad(). This is called for marking a block as bad. >> It invokes nand_do_write_oob(), then calls a higher level accessor >> ecc->write_oob(). >> >> On the other hand, when reading BBM from the OOB, chip->block_bad() >> is called, and nand_block_bad() is the default implementation. This >> function calls a lower level chip->cmdfunc(). If a driver wants to >> re-use nand_block_bad(), it is required to support NAND_CMD_READOOB >> in its cmdfunc(). > > This is part of the basic/mandatory operations that should be supported > by all drivers. My main motivation of this patch is to save NAND_CMD_READOOB implemenation in cmdfunc(). Please look at line 1292 of drivers/mtd/nand/denali.c case NAND_CMD_READOOB: /* TODO: Read OOB data */ break; Currently, this driver can not check BBM at all. If all drivers should support NAND_CMD_READOOB regardless of this patch, my main motivation of this patch will be lost. >> This is strange. If the controller supports >> optimized read operation and the driver has its own ecc->read_oob(), >> it should be able to use it. > > I agree with this one. I guess the idea behind this default > implementation was to avoid reading the whole OOB area, or maybe this > function was implemented before we had ECC support. Anyway, the > overhead should be negligible with your approach. > >> Besides, NAND_CMD_READOOB (0x50) is >> not a real command for large page devices. So, recent drivers may >> not be happy to handle this command. > > Well, that's the whole problem with the ->cmdfunc() hook, even if it's > passed raw NAND command identifiers, these are actually encoding NAND > operations, and not necessarily the exact command that should be sent to > the NAND. I was misunderstanding this. If operations are hooked by higher level accessors and some low-level commands never get chance to be executed, I thought I need not implement them. > See what's done in nand_command_lp(), and how some commands are > actually generating a sequence of 2 commands [1], or how > NAND_CMD_READOOB is transformed into NAND_CMD_READ0 [2]. So, what should I do for denali.c? Maybe, copy the most logic of nand_command_lp() into denali_cmdfunc()? >> if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) >> ofs += mtd->erasesize - mtd->writesize; >> @@ -364,30 +364,26 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; >> >> do { >> - if (chip->options & NAND_BUSWIDTH_16) { >> - chip->cmdfunc(mtd, NAND_CMD_READOOB, >> - chip->badblockpos & 0xFE, page); >> - bad = cpu_to_le16(chip->read_word(mtd)); >> - if (chip->badblockpos & 0x1) >> - bad >>= 8; >> + res = chip->ecc.read_oob(mtd, chip, page); >> + if (!res) { >> + bad = chip->oob_poi[chip->badblockpos]; > > Hm, even if the current code is only testing one byte, I wonder > if we shouldn't test the 2 bytes here when we're dealing with 16bits > NANDs. I was not quite sure about this, so I tried my best to keep the current behavior. >> >> - if (likely(chip->badblockbits == 8)) >> - res = bad != 0xFF; >> - else >> - res = hweight8(bad) < chip->badblockbits; >> - ofs += mtd->writesize; >> - page = (int)(ofs >> chip->page_shift) & chip->pagemask; >> + if (!ret) >> + ret = res; > > Hm, I'm not sure I understand what you're doing here. If res is != 0, > then an error occurred when reading the OOB area and you should > probably return the error code directly instead of trying to read the > OOB from next page. On the other hand, if res is 0, then ret will > always be 0, so I don't think this extra ret variable is needed. My understanding of NAND_BBT_SCAN2NDPAGE is, if at least one page can be read as bad, the corresponding block should be assumed bad. So, I tried to save this case: 1st loop fails to execute chip->ecc.read_oob() 2nd loop succeeds, and the page is marked as bad. nand_default_block_markbad() does a similar thing; it continues even if the first loop fails. If this is a over-care, the code can be more simplified. >> + >> + page++; >> i++; >> - } while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)); >> + } while (chip->bbt_options & NAND_BBT_SCAN2NDPAGE && i < 2); > > A for loop would probably make more sense here, but that's just a > detail. > I just chose less-invasive approach. I do not have a strong opinion about this. >[3]https://github.com/bbrezillon/linux-sunxi/blob/nand-core-rework-v2/include/linux/mtd/nand2.h Is this related to your talk in ELCE 2016? http://events.linuxfoundation.org/sites/events/files/slides/brezillon-nand-framework.pdf Unfortunately I could not attend the last ELCE, but I just skimmed over your slides, and your work looks exciting.
On Wed, 15 Mar 2017 09:55:13 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > Thanks for your review. > > 2017-03-15 5:58 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > On Wed, 15 Mar 2017 02:45:48 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> The nand_default_block_markbad() is the default implementation of > >> chip->block_markbad(). This is called for marking a block as bad. > >> It invokes nand_do_write_oob(), then calls a higher level accessor > >> ecc->write_oob(). > >> > >> On the other hand, when reading BBM from the OOB, chip->block_bad() > >> is called, and nand_block_bad() is the default implementation. This > >> function calls a lower level chip->cmdfunc(). If a driver wants to > >> re-use nand_block_bad(), it is required to support NAND_CMD_READOOB > >> in its cmdfunc(). > > > > This is part of the basic/mandatory operations that should be supported > > by all drivers. > > > My main motivation of this patch is to save NAND_CMD_READOOB > implemenation in cmdfunc(). > > > Please look at line 1292 of drivers/mtd/nand/denali.c > > case NAND_CMD_READOOB: > /* TODO: Read OOB data */ > break; > Yes, I know, and unfortunately that's not the only driver to partially implement the set of operation the core assume to be always supported. > > Currently, this driver can not check BBM at all. > > > If all drivers should support NAND_CMD_READOOB > regardless of this patch, my main motivation of this patch will be lost. Well, I see another reason to move to the ecc->read_oob() approach: some NAND controllers are protecting the BBM with ECC, if you just use ->cmdfunc(READOOB) + ->read_buf() you don't get this ECC protection. > > > >> This is strange. If the controller supports > >> optimized read operation and the driver has its own ecc->read_oob(), > >> it should be able to use it. > > > > I agree with this one. I guess the idea behind this default > > implementation was to avoid reading the whole OOB area, or maybe this > > function was implemented before we had ECC support. Anyway, the > > overhead should be negligible with your approach. > > > >> Besides, NAND_CMD_READOOB (0x50) is > >> not a real command for large page devices. So, recent drivers may > >> not be happy to handle this command. > > > > Well, that's the whole problem with the ->cmdfunc() hook, even if it's > > passed raw NAND command identifiers, these are actually encoding NAND > > operations, and not necessarily the exact command that should be sent to > > the NAND. > > > I was misunderstanding this. > > If operations are hooked by higher level accessors > and some low-level commands never get chance to be executed, > I thought I need not implement them. That's true for some of them (for example ->onfi_set/get_features()), but there's no clear rule saying which commands have to be supported in ->cmdfunc() and which one can be implemented as high-level hooks. Most of the time, I recommend to implement ->cmd_ctrl() and rely on the default ->cmdfunc() implementation, so that, each time a new feature (support for a new NAND operation) is added to the core your driver will support it natively. I know some controllers are not fitting so well in this model, but most of them do. > > > See what's done in nand_command_lp(), and how some commands are > > actually generating a sequence of 2 commands [1], or how > > NAND_CMD_READOOB is transformed into NAND_CMD_READ0 [2]. > > So, what should I do for denali.c? > > Maybe, copy the most logic of nand_command_lp() into denali_cmdfunc()? Ideally, get rid of denali_cmdfunc() and implement denali_cmd_ctrl(), but I'm not sure how feasible this is. > > > > >> if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) > >> ofs += mtd->erasesize - mtd->writesize; > >> @@ -364,30 +364,26 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) > >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; > >> > >> do { > >> - if (chip->options & NAND_BUSWIDTH_16) { > >> - chip->cmdfunc(mtd, NAND_CMD_READOOB, > >> - chip->badblockpos & 0xFE, page); > >> - bad = cpu_to_le16(chip->read_word(mtd)); > >> - if (chip->badblockpos & 0x1) > >> - bad >>= 8; > >> + res = chip->ecc.read_oob(mtd, chip, page); > >> + if (!res) { > >> + bad = chip->oob_poi[chip->badblockpos]; > > > > Hm, even if the current code is only testing one byte, I wonder > > if we shouldn't test the 2 bytes here when we're dealing with 16bits > > NANDs. > > > > I was not quite sure about this, so I tried my best > to keep the current behavior. I'll check in the ONFI spec. > > > > >> > >> - if (likely(chip->badblockbits == 8)) > >> - res = bad != 0xFF; > >> - else > >> - res = hweight8(bad) < chip->badblockbits; > >> - ofs += mtd->writesize; > >> - page = (int)(ofs >> chip->page_shift) & chip->pagemask; > >> + if (!ret) > >> + ret = res; > > > > Hm, I'm not sure I understand what you're doing here. If res is != 0, > > then an error occurred when reading the OOB area and you should > > probably return the error code directly instead of trying to read the > > OOB from next page. On the other hand, if res is 0, then ret will > > always be 0, so I don't think this extra ret variable is needed. > > My understanding of NAND_BBT_SCAN2NDPAGE is, > if at least one page can be read as bad, > the corresponding block should be assumed bad. > > So, I tried to save this case: > 1st loop fails to execute chip->ecc.read_oob() > 2nd loop succeeds, and the page is marked as bad. > > > nand_default_block_markbad() does a similar thing; it continues > even if the first loop fails. > > > If this is a over-care, the code can be more simplified. Actually, I think this is the opposite. Hiding the fact ->read_oob() failed sounds dangerous to me. If we don't return directly we should at least have an error message. > > > > >> + > >> + page++; > >> i++; > >> - } while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)); > >> + } while (chip->bbt_options & NAND_BBT_SCAN2NDPAGE && i < 2); > > > > A for loop would probably make more sense here, but that's just a > > detail. > > > > I just chose less-invasive approach. > I do not have a strong opinion about this. Yep, as I said, I'm just nitpicking, but I think the loop would be clearer with a for loop. > > > >[3]https://github.com/bbrezillon/linux-sunxi/blob/nand-core-rework-v2/include/linux/mtd/nand2.h > > > Is this related to your talk in ELCE 2016? Yep. > http://events.linuxfoundation.org/sites/events/files/slides/brezillon-nand-framework.pdf > > > Unfortunately I could not attend the last ELCE, > but I just skimmed over your slides, and your work looks exciting. As I said at the end of the talk, any help is welcome, so, if you feel your denali driver could benefit from this approach and you have time, don't hesitate to work on it. Regards, Boris
Hi Boris, 2017-03-15 16:55 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Wed, 15 Mar 2017 09:55:13 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> Hi Boris, >> >> Thanks for your review. >> >> 2017-03-15 5:58 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> > On Wed, 15 Mar 2017 02:45:48 +0900 >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> > >> >> The nand_default_block_markbad() is the default implementation of >> >> chip->block_markbad(). This is called for marking a block as bad. >> >> It invokes nand_do_write_oob(), then calls a higher level accessor >> >> ecc->write_oob(). >> >> >> >> On the other hand, when reading BBM from the OOB, chip->block_bad() >> >> is called, and nand_block_bad() is the default implementation. This >> >> function calls a lower level chip->cmdfunc(). If a driver wants to >> >> re-use nand_block_bad(), it is required to support NAND_CMD_READOOB >> >> in its cmdfunc(). I just noticed duplicated efforts for reading BBM. When creating BBT at initialization, functions are called as follows: check_create() create_bbt() scan_block_fast() scan_block_fast() calls high-level API mtd_read_oob() to check BBM. On the other hand, we have nand_block_bad() implemented with lower API. Perhaps, we can merge them. So, do you want to align to the scan_block_fast approach (high level API)?
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index a3c0f47..78c5cd6 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -354,9 +354,9 @@ static void nand_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len) */ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) { - int page, res = 0, i = 0; + int page, res = 0, ret = 0, i = 0; struct nand_chip *chip = mtd_to_nand(mtd); - u16 bad; + u8 bad; if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) ofs += mtd->erasesize - mtd->writesize; @@ -364,30 +364,26 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs) page = (int)(ofs >> chip->page_shift) & chip->pagemask; do { - if (chip->options & NAND_BUSWIDTH_16) { - chip->cmdfunc(mtd, NAND_CMD_READOOB, - chip->badblockpos & 0xFE, page); - bad = cpu_to_le16(chip->read_word(mtd)); - if (chip->badblockpos & 0x1) - bad >>= 8; + res = chip->ecc.read_oob(mtd, chip, page); + if (!res) { + bad = chip->oob_poi[chip->badblockpos]; + + if (likely(chip->badblockbits == 8)) + res = bad != 0xFF; else - bad &= 0xFF; - } else { - chip->cmdfunc(mtd, NAND_CMD_READOOB, chip->badblockpos, - page); - bad = chip->read_byte(mtd); + res = hweight8(bad) < chip->badblockbits; + if (res) + return res; } - if (likely(chip->badblockbits == 8)) - res = bad != 0xFF; - else - res = hweight8(bad) < chip->badblockbits; - ofs += mtd->writesize; - page = (int)(ofs >> chip->page_shift) & chip->pagemask; + if (!ret) + ret = res; + + page++; i++; - } while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)); + } while (chip->bbt_options & NAND_BBT_SCAN2NDPAGE && i < 2); - return res; + return ret; } /**
The nand_default_block_markbad() is the default implementation of chip->block_markbad(). This is called for marking a block as bad. It invokes nand_do_write_oob(), then calls a higher level accessor ecc->write_oob(). On the other hand, when reading BBM from the OOB, chip->block_bad() is called, and nand_block_bad() is the default implementation. This function calls a lower level chip->cmdfunc(). If a driver wants to re-use nand_block_bad(), it is required to support NAND_CMD_READOOB in its cmdfunc(). This is strange. If the controller supports optimized read operation and the driver has its own ecc->read_oob(), it should be able to use it. Besides, NAND_CMD_READOOB (0x50) is not a real command for large page devices. So, recent drivers may not be happy to handle this command. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- At first, I tried to call nand_do_read_oob() from nand_block_bad() in order to make to make things symmetry, but it did not work. chip->select_chip() is handled outside of nand_block_bad(), so nand_do_read_oob() can not be used there. drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-)