[PATCH 0/3] mtd: rawnand: More continuous read fixes

[Miquel Raynal]

Hi Christophe,

christophe.kerello at foss.st.com wrote on Mon, 26 Feb 2024 15:28:59 +0100:

> Hi Miquel,
> 
> On 2/23/24 12:55, Miquel Raynal wrote:
> > Hello,
> > 
> > Following Christophe report I manually inserted many different
> > conditions to test the logic enablingand configuring continuous reads in
> > the core, trying to clarify the core and hopefully fix it for real. I am
> > pretty confident regarding the first patch but a bit more in the fog for
> > the second/third. Even though I'm pretty sure they improve the situation
> > there might still be corner cases I don't have in mind.  
> 
> I have tested the patchset and the issue is fixed, so I will send a tested-by on patch 1.

Great! Thanks!

For now I could not get my hands on a chip with more than one LUN. If
by chance yours has two LUNs (or more), could you please run some
experiments when crossing the LUN boundary?

Anyhow, your Tested-by will be welcome.

> But, I think that there is still an issue using FMC2 and probably others drivers.
> 
> FMC2 driver has 2 modes: polling mode that will called nand_read_page_op and the sequencer mode that is defining its own HW read algorithm.
> 
> The FMC2 sequencer do not support continuous read feature.
> 
> I have added basic logs in nand_do_read_ops. My understanding is that the continuous read feature should be disabled at the end of this function.
> 
> FMC2 polling mode:
> root at stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
> [   41.083132] nand_do_read_ops starts: cont_read.ongoing=1
> [   41.086410] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
> [   41.094797] nand_do_read_ops ends: cont_read.ongoing=0
> [   41.098111] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
> 
> It is OK. In polling mode, con_read.ongoing is set to false before leaving nand_do_read_ops function.
> 
> FMC2 sequencer:
> root at stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
> [   57.143059] nand_do_read_ops starts: cont_read.ongoing=1
> [   57.146370] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
> [   57.154469] nand_do_read_ops ends: cont_read.ongoing=1
> [   57.158020] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
> 
> KO, con_read.ongoing is set to true before leaving nand_do_read_ops function. That means that read_oob can returned wrong data (similar issue as the initial reported issue).
> 
> So, I see 2 ways to fix this issue.
> 
> On framework side by adding a callback (rawnand_disable_cont_reads) that will set to false con_read.ongoing before leaving nand_do_read_ops function.

Can you please trace what happens here? Upon what specific condition
nand_do_read_ops does return with ongoing set to true? This is probably
what needs fixing, but I don't feel like a driver hook is the right
approach.

I was expecting the ongoing boolean to always be reset at the end of
nand_do_read_ops(), I probably missed a scenario. In any case what is
probably needed for the sequencer to work is:

+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3726,6 +3726,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
                }
        }
        nand_deselect_target(chip);
+       chip->cont_read.ongoing = false;
 
        ops->retlen = ops->len - (size_t) readlen;
        if (oob)

> 
> Or
> 
> On FMC2 driver side by disabling the continuous read feature in case of the sequencer is used like it is done in nandsim.c driver.
> Something like:
> if (check_only) {
> 		for (op_id = 0; op_id < op->ninstrs; op_id++) {
> 			instr = &op->instrs[op_id];
> 			if (instr->type == NAND_OP_CMD_INSTR &&
> 			    (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND ||
> 			     instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ))
> 				return -EOPNOTSUPP;
> 		}
> 
> 		return 0;
> 	}

This is a second possible choice if the first one does not give
interesting results. Not my favorite in your case.

Thanks for your help,
Miquèl