* 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).