* nand_op_parser_exec_op should use longest pattern @ 2019-03-29 13:35 Sascha Hauer 2019-03-29 13:57 ` Boris Brezillon 2019-03-29 14:37 ` Stefan Agner 0 siblings, 2 replies; 7+ messages in thread From: Sascha Hauer @ 2019-03-29 13:35 UTC (permalink / raw) To: linux-mtd; +Cc: Stefan Agner, Boris BREZILLON Hi All, I just played with the new exec_op interface for the first time and together with Boris we found a problem in the pattern table parser. The vf610 driver uses this pattern table: static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, NAND_OP_PARSER_PAT_CMD_ELEM(true), NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), NAND_OP_PARSER_PAT_CMD_ELEM(true), NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, NAND_OP_PARSER_PAT_CMD_ELEM(true), NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), NAND_OP_PARSER_PAT_CMD_ELEM(true), NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), ); It has two patterns, one supposed for writing and one for reading. All elements are optional. Now with a typical page read we'll get this: [ 33.932464] nand: ->CMD [0x00] [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] [ 33.941755] nand: ->CMD [0x30] [ 33.945628] nand: ->WAITRDY [max 1 ms] [ 33.949909] nand: DATA_IN [2176 B] Only the first four elements are executed in one go, the fifth is exectuted separately. This is because the pattern table parser finds that the first pattern (supposed for writing) already matches for the first four elements and then uses it instead of realizing that the second pattern matches the whole operation. I have no fix for this, just wanted to let you know. It turned out that in my case for the GPMI nand driver I probably won't need any pattern table. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-03-29 13:35 nand_op_parser_exec_op should use longest pattern Sascha Hauer @ 2019-03-29 13:57 ` Boris Brezillon 2019-03-29 14:37 ` Stefan Agner 1 sibling, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2019-03-29 13:57 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-mtd, Stefan Agner On Fri, 29 Mar 2019 14:35:13 +0100 Sascha Hauer <s.hauer@pengutronix.de> wrote: > Hi All, > > I just played with the new exec_op interface for the first time and > together with Boris we found a problem in the pattern table parser. > > The vf610 driver uses this pattern table: > > static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), > ); > > It has two patterns, one supposed for writing and one for reading. All elements > are optional. Now with a typical page read we'll get this: > > [ 33.932464] nand: ->CMD [0x00] > [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] > [ 33.941755] nand: ->CMD [0x30] > [ 33.945628] nand: ->WAITRDY [max 1 ms] > [ 33.949909] nand: DATA_IN [2176 B] > > Only the first four elements are executed in one go, the fifth is > exectuted separately. This is because the pattern table parser finds > that the first pattern (supposed for writing) already matches for the > first four elements and then uses it instead of realizing that the > second pattern matches the whole operation. > > I have no fix for this, just wanted to let you know. It turned out that > in my case for the GPMI nand driver I probably won't need any pattern > table. This is a completely untested fix, just in case someone has time to test/debug it. --->8--- diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index ddd396e93e32..e0ba9f0b1fdc 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -2165,30 +2165,38 @@ int nand_op_parser_exec_op(struct nand_chip *chip, unsigned int i; while (ctx.subop.instrs < op->instrs + op->ninstrs) { - int ret; + const struct nand_op_parser_pattern *pattern; + struct nand_op_parser_ctx best_ctx; + int ret, best_pattern = -1; for (i = 0; i < parser->npatterns; i++) { - const struct nand_op_parser_pattern *pattern; + struct nand_op_parser_ctx test_ctx = ctx; - pattern = &parser->patterns[i]; - if (!nand_op_parser_match_pat(pattern, &ctx)) + if (!nand_op_parser_match_pat(pattern, &test_ctx)) continue; - nand_op_parser_trace(&ctx); + if (best_pattern >= 0 && + (test_ctx.subop.ninstrs < best_ctx.subop.ninstrs || + (test_ctx.subop.ninstrs == best_ctx.subop.ninstrs && + test_ctx.last_instr_end_off <= best_ctx.last_instr_end_off))) + continue; - if (check_only) - break; + best_pattern = i; + best_ctx = test_ctx; + } + if (best_pattern < 0) { + pr_debug("->exec_op() parser: pattern not found!\n"); + return -ENOTSUPP; + } + + ctx = best_ctx; + nand_op_parser_trace(&ctx); + + if (!check_only) { ret = pattern->exec(chip, &ctx.subop); if (ret) return ret; - - break; - } - - if (i == parser->npatterns) { - pr_debug("->exec_op() parser: pattern not found!\n"); - return -ENOTSUPP; } /* ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-03-29 13:35 nand_op_parser_exec_op should use longest pattern Sascha Hauer 2019-03-29 13:57 ` Boris Brezillon @ 2019-03-29 14:37 ` Stefan Agner 2019-03-30 9:21 ` Boris Brezillon 1 sibling, 1 reply; 7+ messages in thread From: Stefan Agner @ 2019-03-29 14:37 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-mtd, Boris BREZILLON On 29.03.2019 14:35, Sascha Hauer wrote: > Hi All, > > I just played with the new exec_op interface for the first time and > together with Boris we found a problem in the pattern table parser. > > The vf610 driver uses this pattern table: > > static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > NAND_OP_PARSER_PAT_CMD_ELEM(true), > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), > ); > > It has two patterns, one supposed for writing and one for reading. All elements > are optional. Now with a typical page read we'll get this: > > [ 33.932464] nand: ->CMD [0x00] > [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] > [ 33.941755] nand: ->CMD [0x30] > [ 33.945628] nand: ->WAITRDY [max 1 ms] > [ 33.949909] nand: DATA_IN [2176 B] > > Only the first four elements are executed in one go, the fifth is > exectuted separately. This is because the pattern table parser finds > that the first pattern (supposed for writing) already matches for the > first four elements and then uses it instead of realizing that the > second pattern matches the whole operation. Hm, I do not remember noticing that during development. I wonder if it was the case already back then. If yes, it did not seem to have a negative impact on performance compared to the old interface: https://linux-mtd.infradead.narkive.com/qZgEnPsC/patch-v6-0-3-mtd-rawnand-vf610-nfc-make-use-of-exec-op > > I have no fix for this, just wanted to let you know. It turned out that > in my case for the GPMI nand driver I probably won't need any pattern > table. Thanks for bringing it up! Will try it out when I come around. -- Stefan ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-03-29 14:37 ` Stefan Agner @ 2019-03-30 9:21 ` Boris Brezillon 2019-04-08 20:00 ` Stefan Agner 0 siblings, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2019-03-30 9:21 UTC (permalink / raw) To: Stefan Agner; +Cc: Sascha Hauer, linux-mtd, Miquel Raynal +Miquel On Fri, 29 Mar 2019 15:37:56 +0100 Stefan Agner <stefan@agner.ch> wrote: > On 29.03.2019 14:35, Sascha Hauer wrote: > > Hi All, > > > > I just played with the new exec_op interface for the first time and > > together with Boris we found a problem in the pattern table parser. > > > > The vf610 driver uses this pattern table: > > > > static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( > > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > > NAND_OP_PARSER_PAT_CMD_ELEM(true), > > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > > NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), > > NAND_OP_PARSER_PAT_CMD_ELEM(true), > > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), > > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > > NAND_OP_PARSER_PAT_CMD_ELEM(true), > > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > > NAND_OP_PARSER_PAT_CMD_ELEM(true), > > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > > NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), > > ); > > > > It has two patterns, one supposed for writing and one for reading. All elements > > are optional. Now with a typical page read we'll get this: > > > > [ 33.932464] nand: ->CMD [0x00] > > [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] > > [ 33.941755] nand: ->CMD [0x30] > > [ 33.945628] nand: ->WAITRDY [max 1 ms] > > [ 33.949909] nand: DATA_IN [2176 B] > > > > Only the first four elements are executed in one go, the fifth is > > exectuted separately. This is because the pattern table parser finds > > that the first pattern (supposed for writing) already matches for the > > first four elements and then uses it instead of realizing that the > > second pattern matches the whole operation. > > Hm, I do not remember noticing that during development. I wonder if it > was the case already back then. > > If yes, it did not seem to have a negative impact on performance > compared to the old interface: > https://linux-mtd.infradead.narkive.com/qZgEnPsC/patch-v6-0-3-mtd-rawnand-vf610-nfc-make-use-of-exec-op > > > > > I have no fix for this, just wanted to let you know. It turned out that > > in my case for the GPMI nand driver I probably won't need any pattern > > table. > > Thanks for bringing it up! Will try it out when I come around. Here is a new version of the proposed fix that compiles, at least :-). Still not tested tested on a real HW though. --->8--- diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index ddd396e93e32..7a5178b5f13e 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -2131,6 +2131,23 @@ static void nand_op_parser_trace(const struct nand_op_parser_ctx *ctx) } #endif +static int nand_op_parser_cmp_ctx(const struct nand_op_parser_ctx *a, + const struct nand_op_parser_ctx *b) +{ + + if (a->subop.ninstrs < b->subop.ninstrs) + return -1; + else if (a->subop.ninstrs > b->subop.ninstrs) + return 1; + + if (a->subop.last_instr_end_off < b->subop.last_instr_end_off) + return -1; + else if (a->subop.last_instr_end_off > b->subop.last_instr_end_off) + return 1; + + return 0; +} + /** * nand_op_parser_exec_op - exec_op parser * @chip: the NAND chip @@ -2165,30 +2182,38 @@ int nand_op_parser_exec_op(struct nand_chip *chip, unsigned int i; while (ctx.subop.instrs < op->instrs + op->ninstrs) { - int ret; + const struct nand_op_parser_pattern *pattern; + struct nand_op_parser_ctx best_ctx; + int ret, best_pattern = -1; for (i = 0; i < parser->npatterns; i++) { - const struct nand_op_parser_pattern *pattern; + struct nand_op_parser_ctx test_ctx = ctx; pattern = &parser->patterns[i]; - if (!nand_op_parser_match_pat(pattern, &ctx)) + if (!nand_op_parser_match_pat(pattern, &test_ctx)) continue; - nand_op_parser_trace(&ctx); + if (best_pattern >= 0 && + nand_op_parser_cmp_ctx(&test_ctx, &best_ctx) <= 0) + continue; - if (check_only) - break; + best_pattern = i; + best_ctx = test_ctx; + } + if (best_pattern < 0) { + pr_debug("->exec_op() parser: pattern not found!\n"); + return -ENOTSUPP; + } + + ctx = best_ctx; + nand_op_parser_trace(&ctx); + + if (!check_only) { + pattern = &parser->patterns[best_pattern]; ret = pattern->exec(chip, &ctx.subop); if (ret) return ret; - - break; - } - - if (i == parser->npatterns) { - pr_debug("->exec_op() parser: pattern not found!\n"); - return -ENOTSUPP; } /* ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-03-30 9:21 ` Boris Brezillon @ 2019-04-08 20:00 ` Stefan Agner 2019-04-08 21:30 ` Miquel Raynal 2019-04-16 21:22 ` Miquel Raynal 0 siblings, 2 replies; 7+ messages in thread From: Stefan Agner @ 2019-04-08 20:00 UTC (permalink / raw) To: Boris Brezillon; +Cc: Sascha Hauer, linux-mtd, Miquel Raynal Hi Boris, On 30.03.2019 10:21, Boris Brezillon wrote: > +Miquel > > On Fri, 29 Mar 2019 15:37:56 +0100 > Stefan Agner <stefan@agner.ch> wrote: > >> On 29.03.2019 14:35, Sascha Hauer wrote: >> > Hi All, >> > >> > I just played with the new exec_op interface for the first time and >> > together with Boris we found a problem in the pattern table parser. >> > >> > The vf610 driver uses this pattern table: >> > >> > static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( >> > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), >> > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), >> > NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), >> > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), >> > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), >> > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), >> > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), >> > NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), >> > ); >> > >> > It has two patterns, one supposed for writing and one for reading. All elements >> > are optional. Now with a typical page read we'll get this: >> > >> > [ 33.932464] nand: ->CMD [0x00] >> > [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] >> > [ 33.941755] nand: ->CMD [0x30] >> > [ 33.945628] nand: ->WAITRDY [max 1 ms] >> > [ 33.949909] nand: DATA_IN [2176 B] Hm, since I use HW ECC (which uses custom function calls), I do not get such large data reads. But I do have OOB reads from time to time: [ 4.603585] nand: executing subop: [ 4.603616] nand: ->CMD [0x00] [ 4.603646] nand: ->ADDR [5 cyc: 00 08 ea 94 02] [ 4.603673] nand: ->CMD [0x30] [ 4.603700] nand: ->WAITRDY [max 200000 ms] [ 4.603727] nand: DATA_IN [64 B] [ 4.603881] nand: executing subop: [ 4.603912] nand: CMD [0x00] [ 4.603941] nand: ADDR [5 cyc: 00 08 ea 94 02] [ 4.603968] nand: CMD [0x30] [ 4.603994] nand: WAITRDY [max 200000 ms] [ 4.604022] nand: ->DATA_IN [64 B] >> > >> > Only the first four elements are executed in one go, the fifth is >> > exectuted separately. This is because the pattern table parser finds >> > that the first pattern (supposed for writing) already matches for the >> > first four elements and then uses it instead of realizing that the >> > second pattern matches the whole operation. >> >> Hm, I do not remember noticing that during development. I wonder if it >> was the case already back then. >> >> If yes, it did not seem to have a negative impact on performance >> compared to the old interface: >> https://linux-mtd.infradead.narkive.com/qZgEnPsC/patch-v6-0-3-mtd-rawnand-vf610-nfc-make-use-of-exec-op >> >> > >> > I have no fix for this, just wanted to let you know. It turned out that >> > in my case for the GPMI nand driver I probably won't need any pattern >> > table. >> >> Thanks for bringing it up! Will try it out when I come around. > > Here is a new version of the proposed fix that compiles, at > least :-). Still not tested tested on a real HW though. Tested it here, seems to boot a rootfs from flash just fine! The OOB reads look like this now: [ 74.478469] nand: executing subop: [ 74.478500] nand: ->CMD [0x00] [ 74.478529] nand: ->ADDR [5 cyc: 00 08 c0 1d 00] [ 74.478555] nand: ->CMD [0x30] [ 74.478580] nand: ->WAITRDY [max 200000 ms] [ 74.478606] nand: ->DATA_IN [64 B] So the patch below seems to do what we intend. -- Stefan > > --->8--- > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index ddd396e93e32..7a5178b5f13e 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -2131,6 +2131,23 @@ static void nand_op_parser_trace(const struct > nand_op_parser_ctx *ctx) > } > #endif > > +static int nand_op_parser_cmp_ctx(const struct nand_op_parser_ctx *a, > + const struct nand_op_parser_ctx *b) > +{ > + > + if (a->subop.ninstrs < b->subop.ninstrs) > + return -1; > + else if (a->subop.ninstrs > b->subop.ninstrs) > + return 1; > + > + if (a->subop.last_instr_end_off < b->subop.last_instr_end_off) > + return -1; > + else if (a->subop.last_instr_end_off > b->subop.last_instr_end_off) > + return 1; > + > + return 0; > +} > + > /** > * nand_op_parser_exec_op - exec_op parser > * @chip: the NAND chip > @@ -2165,30 +2182,38 @@ int nand_op_parser_exec_op(struct nand_chip *chip, > unsigned int i; > > while (ctx.subop.instrs < op->instrs + op->ninstrs) { > - int ret; > + const struct nand_op_parser_pattern *pattern; > + struct nand_op_parser_ctx best_ctx; > + int ret, best_pattern = -1; > > for (i = 0; i < parser->npatterns; i++) { > - const struct nand_op_parser_pattern *pattern; > + struct nand_op_parser_ctx test_ctx = ctx; > > pattern = &parser->patterns[i]; > - if (!nand_op_parser_match_pat(pattern, &ctx)) > + if (!nand_op_parser_match_pat(pattern, &test_ctx)) > continue; > > - nand_op_parser_trace(&ctx); > + if (best_pattern >= 0 && > + nand_op_parser_cmp_ctx(&test_ctx, &best_ctx) <= 0) > + continue; > > - if (check_only) > - break; > + best_pattern = i; > + best_ctx = test_ctx; > + } > > + if (best_pattern < 0) { > + pr_debug("->exec_op() parser: pattern not found!\n"); > + return -ENOTSUPP; > + } > + > + ctx = best_ctx; > + nand_op_parser_trace(&ctx); > + > + if (!check_only) { > + pattern = &parser->patterns[best_pattern]; > ret = pattern->exec(chip, &ctx.subop); > if (ret) > return ret; > - > - break; > - } > - > - if (i == parser->npatterns) { > - pr_debug("->exec_op() parser: pattern not found!\n"); > - return -ENOTSUPP; > } > > /* ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-04-08 20:00 ` Stefan Agner @ 2019-04-08 21:30 ` Miquel Raynal 2019-04-16 21:22 ` Miquel Raynal 1 sibling, 0 replies; 7+ messages in thread From: Miquel Raynal @ 2019-04-08 21:30 UTC (permalink / raw) To: Stefan Agner; +Cc: Sascha Hauer, Boris Brezillon, linux-mtd Hi Stefan, Stefan Agner <stefan@agner.ch> wrote on Mon, 08 Apr 2019 22:00:18 +0200: > Hi Boris, > > On 30.03.2019 10:21, Boris Brezillon wrote: > > +Miquel > > > > On Fri, 29 Mar 2019 15:37:56 +0100 > > Stefan Agner <stefan@agner.ch> wrote: > > > >> On 29.03.2019 14:35, Sascha Hauer wrote: > >> > Hi All, > >> > > >> > I just played with the new exec_op interface for the first time and > >> > together with Boris we found a problem in the pattern table parser. > >> > > >> > The vf610 driver uses this pattern table: > >> > > >> > static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER( > >> > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), > >> > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > >> > NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX), > >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), > >> > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), > >> > NAND_OP_PARSER_PATTERN(vf610_nfc_cmd, > >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), > >> > NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), > >> > NAND_OP_PARSER_PAT_CMD_ELEM(true), > >> > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > >> > NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)), > >> > ); > >> > > >> > It has two patterns, one supposed for writing and one for reading. All elements > >> > are optional. Now with a typical page read we'll get this: > >> > > >> > [ 33.932464] nand: ->CMD [0x00] > >> > [ 33.936338] nand: ->ADDR [5 cyc: 00 00 00 0a 00] > >> > [ 33.941755] nand: ->CMD [0x30] > >> > [ 33.945628] nand: ->WAITRDY [max 1 ms] > >> > [ 33.949909] nand: DATA_IN [2176 B] > > Hm, since I use HW ECC (which uses custom function calls), I do not get > such large data reads. But I do have OOB reads from time to time: > > [ 4.603585] nand: executing subop: > [ 4.603616] nand: ->CMD [0x00] > [ 4.603646] nand: ->ADDR [5 cyc: 00 08 ea 94 02] > [ 4.603673] nand: ->CMD [0x30] > [ 4.603700] nand: ->WAITRDY [max 200000 ms] > [ 4.603727] nand: DATA_IN [64 B] > [ 4.603881] nand: executing subop: > [ 4.603912] nand: CMD [0x00] > [ 4.603941] nand: ADDR [5 cyc: 00 08 ea 94 02] > [ 4.603968] nand: CMD [0x30] > [ 4.603994] nand: WAITRDY [max 200000 ms] > [ 4.604022] nand: ->DATA_IN [64 B] > > > > >> > > >> > Only the first four elements are executed in one go, the fifth is > >> > exectuted separately. This is because the pattern table parser finds > >> > that the first pattern (supposed for writing) already matches for the > >> > first four elements and then uses it instead of realizing that the > >> > second pattern matches the whole operation. > >> > >> Hm, I do not remember noticing that during development. I wonder if it > >> was the case already back then. > >> > >> If yes, it did not seem to have a negative impact on performance > >> compared to the old interface: > >> https://linux-mtd.infradead.narkive.com/qZgEnPsC/patch-v6-0-3-mtd-rawnand-vf610-nfc-make-use-of-exec-op > >> > >> > > >> > I have no fix for this, just wanted to let you know. It turned out that > >> > in my case for the GPMI nand driver I probably won't need any pattern > >> > table. > >> > >> Thanks for bringing it up! Will try it out when I come around. > > > > Here is a new version of the proposed fix that compiles, at > > least :-). Still not tested tested on a real HW though. > > Tested it here, seems to boot a rootfs from flash just fine! > > The OOB reads look like this now: > [ 74.478469] nand: executing subop: > [ 74.478500] nand: ->CMD [0x00] > [ 74.478529] nand: ->ADDR [5 cyc: 00 08 c0 1d 00] > [ 74.478555] nand: ->CMD [0x30] > [ 74.478580] nand: ->WAITRDY [max 200000 ms] > [ 74.478606] nand: ->DATA_IN [64 B] > > So the patch below seems to do what we intend. Great! Stefan, I know Boris is busy, can you please send the patch? Thanks! Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: nand_op_parser_exec_op should use longest pattern 2019-04-08 20:00 ` Stefan Agner 2019-04-08 21:30 ` Miquel Raynal @ 2019-04-16 21:22 ` Miquel Raynal 1 sibling, 0 replies; 7+ messages in thread From: Miquel Raynal @ 2019-04-16 21:22 UTC (permalink / raw) To: Stefan Agner; +Cc: Sascha Hauer, Boris Brezillon, linux-mtd Hi Stefan, > >> > I have no fix for this, just wanted to let you know. It turned out that > >> > in my case for the GPMI nand driver I probably won't need any pattern > >> > table. > >> > >> Thanks for bringing it up! Will try it out when I come around. > > > > Here is a new version of the proposed fix that compiles, at > > least :-). Still not tested tested on a real HW though. > > Tested it here, seems to boot a rootfs from flash just fine! > > The OOB reads look like this now: > [ 74.478469] nand: executing subop: > [ 74.478500] nand: ->CMD [0x00] > [ 74.478529] nand: ->ADDR [5 cyc: 00 08 c0 1d 00] > [ 74.478555] nand: ->CMD [0x30] > [ 74.478580] nand: ->WAITRDY [max 200000 ms] > [ 74.478606] nand: ->DATA_IN [64 B] > > So the patch below seems to do what we intend. Would you mind sending a proper patch with the below change? You can credit Boris with a suggested-by. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-16 21:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-29 13:35 nand_op_parser_exec_op should use longest pattern Sascha Hauer 2019-03-29 13:57 ` Boris Brezillon 2019-03-29 14:37 ` Stefan Agner 2019-03-30 9:21 ` Boris Brezillon 2019-04-08 20:00 ` Stefan Agner 2019-04-08 21:30 ` Miquel Raynal 2019-04-16 21:22 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).