by Sascha Hauer

[permalink] [raw]

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

On Mon, May 13, 2024 at 09:32:56AM +0200, Miquel Raynal wrote:
>
> [email protected] 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.

Ok.

> >
> > > +
> > > + 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."

Ok.

>
> And this is actually very wrong.
>
> You should call wait_op_done() instead.

No, I don't think so. wait_op_done() is called to wait for the interrupt
of the controller indicating a basic operation is done. A basic operation
can be that a command is being sent or an address byte has been sent to
the chip during an address cycle.

With this arbitrary example:

struct nand_op_instr instrs[] = {
NAND_OP_CMD(NAND_CMD_READ0, 0),
NAND_OP_ADDR(4, addrs, 0),
NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
NAND_COMMON_TIMING_NS(conf, tRR_min)),
NAND_OP_DATA_IN(len, buf, 0),
};

I'll call wait_op_done() once for NAND_CMD_READ0, four times for the NAND_OP_ADDR,
then once again for the NAND_CMD_READSTART command and four times (on i.MX27, once
per subpage) for the NAND_OP_DATA_IN operation. Calling wait_op_done() for the
NAND_OP_WAIT_RDY operation woul only timeout because there is no operation in flight
currently.

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 |

2024-05-14 10:46:20

by Miquel Raynal

[permalink] [raw]

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

Hi Sascha,

> > > > + 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."
>
> Ok.
>
> >
> > And this is actually very wrong.
> >
> > You should call wait_op_done() instead.
>
> No, I don't think so. wait_op_done() is called to wait for the interrupt
> of the controller indicating a basic operation is done. A basic operation
> can be that a command is being sent or an address byte has been sent to
> the chip during an address cycle.
>
> With this arbitrary example:
>
> struct nand_op_instr instrs[] = {
> NAND_OP_CMD(NAND_CMD_READ0, 0),
> NAND_OP_ADDR(4, addrs, 0),
> NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
> NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
> NAND_COMMON_TIMING_NS(conf, tRR_min)),
> NAND_OP_DATA_IN(len, buf, 0),
> };
>
> I'll call wait_op_done() once for NAND_CMD_READ0, four times for the NAND_OP_ADDR,
> then once again for the NAND_CMD_READSTART command and four times (on i.MX27, once
> per subpage) for the NAND_OP_DATA_IN operation. Calling wait_op_done() for the
> NAND_OP_WAIT_RDY operation woul only timeout because there is no operation in flight
> currently.

Ah, ok, makes sense as well. If you want (this is not mandatory) you
may want to rename the function wait_instr_done() to fit the core's
naming: an operation being an aggregate of instructions/cycles.

Thanks,
Miquèl