Re: nand_op_parser_exec_op should use longest pattern

[Miquel Raynal <miquel.raynal@bootlin.com>]

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/