Re: [PATCH v2 2/3] mtd: nand: mxc_nand: implement exec_op

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Miquel,

On Mon, May 13, 2024 at 09:32:56AM +0200, Miquel Raynal wrote:
> 
> miquel.raynal@bootlin.com wrote on Mon, 13 May 2024 09:19:02 +0200:
> 
> > Hi Sascha,
> > 
> > > @@ -1717,9 +1465,111 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
> > >  	return host->devtype_data->setup_interface(chip, chipnr, conf);
> > >  }
> > >  
> > > +static int mxcnd_exec_op(struct nand_chip *chip,
> > > +			 const struct nand_operation *op,
> > > +			 bool check_only)
> > > +{
> > > +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> > > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > > +	int i, j, buf_len;
> > > +	void *buf_read = NULL;
> > > +	const void *buf_write = NULL;
> > > +	const struct nand_op_instr *instr;
> > > +	bool readid = false;
> > > +	bool statusreq = false;
> > > +
> > > +	dev_dbg(host->dev, "%s: %d instructions\n", __func__, op->ninstrs);  
> > 
> > Maybe you want to get rid of this debug line.
> > 
> > > +
> > > +	if (check_only)
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < op->ninstrs; i++) {
> > > +		instr = &op->instrs[i];
> > > +
> > > +		nand_op_trace("  ", instr);
> > > +
> > > +		switch (instr->type) {
> > > +		case NAND_OP_WAITRDY_INSTR:
> > > +			/*
> > > +			 * NFC handles R/B internally. Therefore, this function
> > > +			 * always returns status as ready.  
> > 
> > This is no longer a standalone function, maybe:
> > 
> > "The controller handles the R/B pin internally, therefore there is
> > nothing to do here."
> 
> And this is actually very wrong.
> 
> You should call wait_op_done() instead.
> 
> > 
> > > +			 */
> > > +			break;
> > > +		case NAND_OP_CMD_INSTR:
> > > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> > > +				host->devtype_data->send_page(mtd, NFC_INPUT);
> 
> Actually this is not the right place. You should trigger the transfer
> from controller SRAM to NAND (and the other way around) in the
> NAND_OP_DATA_OUT_INSTR case.
> 
> Here you should just call ->send_cmd.

I tried to get away here with using the standard nand_write_page_raw()
function. This does multiple NAND_OP_DATA_OUT_INSTR ops before finally
sending a NAND_CMD_PAGEPROG command. With software BCH ECC I collect all
data being written in memory and copy it to the controller SRAM en bloc.
I have to do this after the final NAND_OP_DATA_OUT_INSTR op. During a
NAND_OP_DATA_OUT_INSTR I don't know if this is going to be the last one
or if other NAND_OP_DATA_OUT_INSTR ops follow, so I do the copy to SRAM
right before a NAND_CMD_PAGEPROG.

I could move the ->send_page() call to NAND_OP_DATA_OUT_INSTR, but then
I have to overwrite the ecc->write_page_raw() hook. Also it must be
clear that the NAND core will never do multiple NAND_OP_DATA_OUT_INSTR
when programming a page.

Side note: I decided to collect the page data in a memory buffer rather
than in the controller SRAM directly because it seemed too complicated
and error prone to find the correct offset in SRAM for random column
addresses. Also the SRAM can only do word and halfword accesses, so I
additionally would have to emulate byte accesses with read-modify-write
halfword accesses. While certainly doable I'd like to defer this to a
future optimisation exercise.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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/