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

[Miquel Raynal]

miquel.raynal at 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.

> > +
> > +			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> > +
> > +			if (instr->ctx.cmd.opcode == NAND_CMD_READID)
> > +				readid = true;
> > +			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
> > +				statusreq = true;
> > +
> > +			break;
> > +		case NAND_OP_ADDR_INSTR:
> > +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> > +				bool islast = j == instr->ctx.addr.naddrs - 1;
> > +				host->devtype_data->send_addr(host, instr->ctx.addr.addrs[j], islast);
> > +			}
> > +			break;
> > +		case NAND_OP_DATA_OUT_INSTR:
> > +			buf_write = instr->ctx.data.buf.out;
> > +			buf_len = instr->ctx.data.len;
> > +
> > +			memcpy32_toio(host->main_area0, buf_write, buf_len);
> > +			if (chip->oob_poi)
> > +				copy_spare(mtd, false, chip->oob_poi);  
> 
> This copy should not be needed. It should be in your page accessors if
> needed.
> 
> > +
> > +			break;
> > +		case NAND_OP_DATA_IN_INSTR:
> > +
> > +			buf_read = instr->ctx.data.buf.in;
> > +			buf_len = instr->ctx.data.len;
> > +
> > +			if (readid) {
> > +				host->devtype_data->send_read_id(host);
> > +				readid = false;
> > +
> > +				memcpy32_fromio(host->data_buf, host->main_area0, buf_len * 2);
> > +
> > +				if (chip->options & NAND_BUSWIDTH_16) {
> > +					u8 *bufr = buf_read;
> > +					u16 *bufw = host->data_buf;
> > +					for (j = 0; j < buf_len; j++)
> > +						bufr[j] = bufw[j];
> > +				} else {
> > +					memcpy(buf_read, host->data_buf, buf_len);
> > +				}
> > +				break;
> > +			}
> > +
> > +			if (statusreq) {
> > +				*(u8*)buf_read = host->devtype_data->get_dev_status(host);
> > +				statusreq = false;
> > +				break;
> > +			}
> > +
> > +			host->devtype_data->read_page(chip);
> > +
> > +			if (IS_ALIGNED(buf_len, 4)) {
> > +				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> > +			} else {
> > +				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> > +				memcpy(buf_read, host->data_buf, buf_len);
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> Otherwise I'm very happy with the look.
> 
> Thanks,
> Miquèl


Thanks,
Miquèl