[RFC 04/12] mtd: nand: add ->exec_op() implementation
Miquel RAYNAL
miquel.raynal at free-electrons.com
Mon Nov 6 06:09:40 PST 2017
Hi Boris,
> > Introduce the new way to control the NAND controller drivers by
> > implementing the ->exec_op() core helpers and allowing new drivers
> > to use it instead of relying on ->cmd_ctrl(), ->cmdfunc() and
> > ->read/write_byte/word/buf().
>
> "
> Introduce a new interface to instruct NAND controllers to send
> specific NAND operations. The new interface takes the form of a
> single method called ->exec_op(). This method is designed to replace
> ->cmd_ctrl(), ->cmdfunc() and ->read/write_byte/word/buf() hooks.
> "
Changed.
>
> >
> > The logic is now to send to the controller driver a list of
> > instructions.
>
> "
> ->exec_op() is passed a set of instructions describing the operation
> to execute. Each instruction has a type (ADDR, CMD, CYCLE, WAITRDY)
> and delay. The type is directly matching the description of NAND
> commands in various NAND datasheet and standards (ONFI, JEDEC), the
> delay is here to help simple controllers wait enough time between each
> instruction. Advanced controllers with integrated timings control can
> ignore these delays.
> "
Changed with s/CYCLE/DATA/.
>
> > The driver shall use the parser added by this commit to
> > get the matching hook if any.
>
> That's true only for advanced controllers. For those who are executing
> each instruction individually (like the gpio-nand driver, or any old
> style driver where each ADDR/CMD/DATA cycle is controlled
> independently) this is not needed, and who add extra complexity for no
> real gain.
Mention about advanced vs dump controllers added.
>
> > The instructions may be split by the
> > parser in order to comply with the controller constraints filled in
> > an array of supported patterns.
>
> Given only this description it's hard to tell what this parser is and
> why it's needed. I think a real example who be helpful to better
> understand what this is.
Added an example.
>
> >
> > Various helpers are also added to ease NAND controller drivers
> > writing.
> >
> > This new interface should really ease the support of new vendor
> > specific instructions.
>
> Actually, it's more than easing support for vendor specific
> operations, it allows the core to check whether the NAND controller
> will be able to execute a specific operation or not, which before
> that was impossible. It doesn't mean all controllers will magically
> support all kind of NAND operations, but at least we'll know when it
> doesn't.
Added a note about that.
>
> >
> > Suggested-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal at free-electrons.com>
> > ---
> > drivers/mtd/nand/denali.c | 93 +++-
> > drivers/mtd/nand/nand_base.c | 949
> > +++++++++++++++++++++++++++++++++++++++--
> > drivers/mtd/nand/nand_hynix.c | 91 +++-
> > drivers/mtd/nand/nand_micron.c | 32 +-
> > include/linux/mtd/rawnand.h | 366 +++++++++++++++- 5 files
> > changed, 1448 insertions(+), 83 deletions(-)
> >
...
> > @@ -665,11 +663,22 @@ static void denali_oob_xfer(struct mtd_info
> > *mtd, struct nand_chip *chip, int i, pos, len;
> >
> > /* BBM at the beginning of the OOB area */
> > - chip->cmdfunc(mtd, start_cmd, writesize, page);
> > - if (write)
> > - chip->write_buf(mtd, bufpoi, oob_skip);
> > - else
> > - chip->read_buf(mtd, bufpoi, oob_skip);
> > + if (chip->exec_op) {
> > + if (write)
> > + nand_prog_page_begin_op(chip, page,
> > writesize, bufpoi,
> > + oob_skip);
> > + else
> > + nand_read_page_op(chip, page, writesize,
> > bufpoi,
> > + oob_skip);
> > + } else {
> > + if (write) {
> > + chip->cmdfunc(mtd, NAND_CMD_SEQIN,
> > writesize, page);
> > + chip->write_buf(mtd, bufpoi, oob_skip);
> > + } else {
> > + chip->cmdfunc(mtd, NAND_CMD_READ0,
> > writesize, page);
> > + chip->read_buf(mtd, bufpoi, oob_skip);
> > + }
> > + }
>
> Why do we have to keep the ->cmdfunc() logic? nand_prog_page_xxx()
> already abstracts this for us, right?
Right! Changed here and after.
>
> > bufpoi += oob_skip;
> >
> > /* OOB ECC */
> > @@ -682,30 +691,67 @@ static void denali_oob_xfer(struct mtd_info
> > *mtd, struct nand_chip *chip, else if (pos + len > writesize)
> > len = writesize - pos;
> >
> > - chip->cmdfunc(mtd, rnd_cmd, pos, -1);
> > - if (write)
> > - chip->write_buf(mtd, bufpoi, len);
> > - else
> > - chip->read_buf(mtd, bufpoi, len);
> > - bufpoi += len;
> > - if (len < ecc_bytes) {
> > - len = ecc_bytes - len;
> > - chip->cmdfunc(mtd, rnd_cmd, writesize +
> > oob_skip, -1);
> > + if (chip->exec_op) {
> > if (write)
> > - chip->write_buf(mtd, bufpoi, len);
> > + nand_change_write_column_op(
> > + chip, pos, bufpoi, len,
> > false);
>
> Nitpick, but can you try to align things like that:
>
> nand_change_write_column_op(chip, pos,
> bufpoi,
> len, false);
Changed here and after.
>
> > else
> > + nand_change_read_column_op(
> > + chip, pos, bufpoi, len,
> > false);
>
> Ditto.
>
> > + } else {
> > + if (write) {
> > + chip->cmdfunc(mtd, NAND_CMD_RNDIN,
> > pos, -1);
> > + chip->write_buf(mtd, bufpoi, len);
> > + } else {
> > + chip->cmdfunc(mtd,
> > NAND_CMD_RNDOUT, pos, -1); chip->read_buf(mtd, bufpoi, len);
> > + }
> > + }
>
> Smae here: I don't think we need to support both ->cmdfunc() and
> nand_change_read/write_column().
>
> > + bufpoi += len;
> > + if (len < ecc_bytes) {
> > + len = ecc_bytes - len;
> > + if (chip->exec_op) {
> > + if (write)
> > +
> > nand_change_write_column_op(
> > + chip, writesize +
> > oob_skip,
> > + bufpoi, len,
> > false);
> > + else
> > + nand_change_read_column_op(
> > + chip, writesize +
> > oob_skip,
> > + bufpoi, len,
> > false);
> > + } else {
> > + if (write) {
> > + chip->cmdfunc(mtd,
> > NAND_CMD_RNDIN,
> > + writesize +
> > oob_skip, -1);
> > + chip->write_buf(mtd,
> > bufpoi, len);
> > + } else {
> > + chip->cmdfunc(mtd,
> > NAND_CMD_RNDOUT,
> > + writesize +
> > oob_skip, -1);
> > + chip->read_buf(mtd,
> > bufpoi, len);
> > + }
> > + .
>
> Ditto.
>
> > bufpoi += len;
> > }
> > }
> >
> > /* OOB free */
> > len = oobsize - (bufpoi - chip->oob_poi);
> > - chip->cmdfunc(mtd, rnd_cmd, size - len, -1);
> > - if (write)
> > - chip->write_buf(mtd, bufpoi, len);
> > - else
> > - chip->read_buf(mtd, bufpoi, len);
> > + if (chip->exec_op) {
> > + if (write)
> > + nand_change_write_column_op(chip, size -
> > len, bufpoi,
> > + len, false);
> > + else
> > + nand_change_read_column_op(chip, size -
> > len, bufpoi,
> > + len, false);
> > + } else {
> > + if (write) {
> > + chip->cmdfunc(mtd, NAND_CMD_RNDIN, size -
> > len, -1);
> > + chip->write_buf(mtd, bufpoi, len);
> > + } else {
> > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, size -
> > len, -1);
> > + chip->read_buf(mtd, bufpoi, len);
> > + }
> > + }
> > }
> >
> > static int denali_read_page_raw(struct mtd_info *mtd, struct
> > nand_chip *chip, @@ -803,6 +849,9 @@ static int
> > denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> > denali_oob_xfer(mtd, chip, page, 1);
> >
> > + if (chip->exec_op)
> > + return nand_prog_page_end_op(chip);
> > +
> > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> > status = chip->waitfunc(mtd, chip);
>
> All denali changes in this patch should actually be moved to patch 1.
Ok.
>
> >
> > diff --git a/drivers/mtd/nand/nand_base.c
> > b/drivers/mtd/nand/nand_base.c index bef20e06f0db..737f19bd2f83
> > 100644 --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -814,7 +814,7 @@ static void nand_ccs_delay(struct nand_chip
> > *chip)
> > * Wait tCCS_min if it is correctly defined, otherwise
> > wait 500ns
> > * (which should be safe for all NANDs).
> > */
> > - if (chip->data_interface.timings.sdr.tCCS_min)
> > + if (&chip->data_interface.timings.sdr.tCCS_min)
>
> This condition is always true. I think this should be replaced by:
>
> if (chip->setup_data_interface)
Yes.
>
> And BTW, it should go in patch 3.
Ok.
>
> > ndelay(chip->data_interface.timings.sdr.tCCS_min /
> > 1000); else
> > ndelay(500);
> > @@ -1233,6 +1233,118 @@ static int nand_init_data_interface(struct
> > nand_chip *chip) }
> >
> > /**
> > + * nand_fill_column_cycles - fill the column fields on an address
> > array
> > + * @chip: The NAND chip
> > + * @addrs: Array of address cycles to fill
> > + * @offset_in_page: The offset in the page
> > + *
> > + * Fills the first or the two first bytes of the @addrs field
> > depending
> > + * on the NAND bus width and the page size.
> > + */
> > +int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs,
> > + unsigned int offset_in_page)
> > +{
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > + /*
> > + * The offset in page is expressed in bytes, if the NAND
> > bus is 16-bit
> > + * wide, then it must be divided by 2.
> > + */
> > + if (chip->options & NAND_BUSWIDTH_16) {
> > + if (offset_in_page % 2) {
> > + WARN_ON(true);
> > + return -EINVAL;
> > + }
> > +
> > + offset_in_page /= 2;
> > + }
> > +
> > + addrs[0] = offset_in_page;
> > +
> > + /* Small pages use 1 cycle for the columns, while large
> > page need 2 */
> > + if (mtd->writesize <= 512)
> > + return 1;
> > +
> > + addrs[1] = offset_in_page >> 8;
> > +
> > + return 2;
> > +}
> > +EXPORT_SYMBOL_GPL(nand_fill_column_cycles);
> > +
> > +static int nand_sp_exec_read_page_op(struct nand_chip *chip,
> > unsigned int page,
> > + unsigned int offset_in_page,
> > void *buf,
> > + unsigned int len)
> > +{
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + const struct nand_sdr_timings *sdr =
> > + nand_get_sdr_timings(&chip->data_interface);
> > + u8 addrs[4];
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_CMD_READ0, 0),
> > + NAND_OP_ADDR(3, addrs, sdr->tWB_max),
> > + NAND_OP_WAIT_RDY(sdr->tR_max, sdr->tRR_min),
> > + NAND_OP_DATA_IN(len, buf, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + /* Drop the DATA_OUT instruction if len is set to 0. */
> > + if (!len)
> > + op.ninstrs--;
> > +
> > + if (offset_in_page >= mtd->writesize)
> > + instrs[0].cmd.opcode = NAND_CMD_READOOB;
> > + else if (offset_in_page >= 256)
> > + instrs[0].cmd.opcode = NAND_CMD_READ1;
> > +
> > + if (nand_fill_column_cycles(chip, addrs, offset_in_page) <
> > 0)
>
> Why not returning the retcode of nand_fill_column_cycles() directly?
>
> ret = nand_fill_column_cycles(chip, addrs, offset_in_page)
> if (ret < 0)
> return ret;
Changed.
>
> > + return -EINVAL;
> > +
> > + addrs[1] = page;
> > + addrs[2] = page >> 8;
> > +
> > + if (chip->options & NAND_ROW_ADDR_3) {
> > + addrs[3] = page >> 16;
> > + instrs[1].addr.naddrs++;
> > + }
> > +
> > + return nand_exec_op(chip, &op);
> > +}
> > +
> > +static int nand_lp_exec_read_page_op(struct nand_chip *chip,
> > unsigned int page,
> > + unsigned int offset_in_page,
> > void *buf,
> > + unsigned int len)
> > +{
> > + const struct nand_sdr_timings *sdr =
> > + nand_get_sdr_timings(&chip->data_interface);
> > + u8 addrs[5];
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_CMD_READ0, 0),
> > + NAND_OP_ADDR(4, addrs, 0),
> > + NAND_OP_CMD(NAND_CMD_READSTART, sdr->tWB_max),
> > + NAND_OP_WAIT_RDY(sdr->tR_max, sdr->tRR_min),
> > + NAND_OP_DATA_IN(len, buf, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + /* Drop the DATA_IN instruction if len is set to 0. */
> > + if (!len)
> > + op.ninstrs--;
> > +
> > + if (nand_fill_column_cycles(chip, addrs, offset_in_page))
>
> Should be
>
> if (nand_fill_column_cycles(chip, addrs, offset_in_page) < 0)
+ use of ret =, if (ret < 0)...
>
> > + return -EINVAL;
> > +
> > + addrs[2] = page;
> > + addrs[3] = page >> 8;
> > +
> > + if (chip->options & NAND_ROW_ADDR_3) {
> > + addrs[4] = page >> 16;
> > + instrs[1].addr.naddrs++;
> > + }
> > +
> > + return nand_exec_op(chip, &op);
> > +}
> > +
> > +/**
> > * nand_read_page_op - Do a READ PAGE operation
> > * @chip: The NAND chip
> > * @page: page to read
> > @@ -1246,17 +1358,26 @@ static int nand_init_data_interface(struct
> > nand_chip *chip)
> > * Returns 0 for success or negative error code otherwise
> > */
> > int nand_read_page_op(struct nand_chip *chip, unsigned int page,
> > - unsigned int column, void *buf, unsigned int
> > len)
> > + unsigned int offset_in_page, void *buf,
> > unsigned int len)
>
> You should change the parameter name in patch 1 since this function is
> introduced there.
Ok, done in other places as well.
>
> > {
> > struct mtd_info *mtd = nand_to_mtd(chip);
> >
> > if (len && !buf)
> > return -EINVAL;
> >
> > - if (column + len > mtd->writesize + mtd->oobsize)
> > + if (offset_in_page + len > mtd->writesize + mtd->oobsize)
> > return -EINVAL;
> >
> > - chip->cmdfunc(mtd, NAND_CMD_READ0, column, page);
> > + if (chip->exec_op) {
> > + if (mtd->writesize > 512)
> > + return nand_lp_exec_read_page_op(
> > + chip, page, offset_in_page, buf,
> > len); +
> > + return nand_sp_exec_read_page_op(chip, page,
> > offset_in_page,
> > + buf, len);
> > + }
> > +
> > + chip->cmdfunc(mtd, NAND_CMD_READ0, offset_in_page, page);
> > if (len)
> > chip->read_buf(mtd, buf, len);
> >
> > @@ -1286,6 +1407,25 @@ static int nand_read_param_page_op(struct
> > nand_chip *chip, u8 page, void *buf, if (len && !buf)
> > return -EINVAL;
> >
> > + if (chip->exec_op) {
> > + const struct nand_sdr_timings *sdr =
> > +
> > nand_get_sdr_timings(&chip->data_interface);
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_CMD_PARAM, 0),
> > + NAND_OP_ADDR(1, &page, sdr->tWB_max),
> > + NAND_OP_WAIT_RDY(sdr->tR_max,
> > sdr->tRR_min),
> > + NAND_OP_8BIT_DATA_IN(len, buf, 0),
> > + };
> > + struct nand_operation op =
> > + NAND_OPERATION(instrs);
> > +
> > + /* Drop the DATA_IN instruction if len is set to
> > 0. */
> > + if (!len)
> > + op.ninstrs--;
> > +
> > + return nand_exec_op(chip, &op);
> > + }
> > +
> > chip->cmdfunc(mtd, NAND_CMD_PARAM, page, -1);
> > for (i = 0; i < len; i++)
> > p[i] = chip->read_byte(mtd);
>
> [...]
>
> > +
> > /**
> > * nand_prog_page_begin_op - starts a PROG PAGE operation
> > * @chip: The NAND chip
> > @@ -1371,7 +1608,7 @@ EXPORT_SYMBOL_GPL(nand_read_oob_op);
> > * Returns 0 for success or negative error code otherwise
> > */
> > int nand_prog_page_begin_op(struct nand_chip *chip, unsigned int
> > page,
> > - unsigned int column, const void *buf,
> > + unsigned int offset_in_page, const
> > void *buf, unsigned int len)
> > {
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > @@ -1379,10 +1616,14 @@ int nand_prog_page_begin_op(struct
> > nand_chip *chip, unsigned int page, if (len && !buf)
> > return -EINVAL;
> >
> > - if (column + len > mtd->writesize + mtd->oobsize)
> > + if (offset_in_page + len > mtd->writesize + mtd->oobsize)
> > return -EINVAL;
> >
> > - chip->cmdfunc(mtd, NAND_CMD_SEQIN, column, page);
> > + if (chip->exec_op)
> > + return nand_exec_prog_page_op(
> > + chip, page, offset_in_page, buf, len,
> > false);
>
> Param alignment please.
Done
>
> > +
> > + chip->cmdfunc(mtd, NAND_CMD_SEQIN, offset_in_page, page);
> >
> > if (buf)
> > chip->write_buf(mtd, buf, len);
> > @@ -1405,6 +1646,19 @@ int nand_prog_page_end_op(struct nand_chip
> > *chip) struct mtd_info *mtd = nand_to_mtd(chip);
> > int status;
> >
> > + if (chip->exec_op) {
> > + const struct nand_sdr_timings *sdr =
> > +
> > nand_get_sdr_timings(&chip->data_interface);
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_CMD_PAGEPROG,
> > sdr->tWB_max),
> > + NAND_OP_WAIT_RDY(sdr->tPROG_max, 0),
> > + };
> > + struct nand_operation op =
> > + NAND_OPERATION(instrs);
> > +
> > + return nand_exec_op(chip, &op);
> > + }
> > +
> > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> >
> > status = chip->waitfunc(mtd, chip);
[...]
> > +/**
> > + * nand_op_parser_must_split_instr - Checks if an instruction must
> > be split
> > + * @pat: the parser pattern that match
> > + * @instr: the instruction array to check
> > + * @start_offset: the offset from which to start in the first
> > instruction of the
> > + * @instr array
> > + *
> > + * Some NAND controllers are limited and cannot send X address
> > cycles with a
> > + * unique operation, or cannot read/write more than Y bytes at the
> > same time.
> > + * In this case, reduce the scope of this set of operation, and
> > trigger another
> > + * instruction with the rest.
>
> "
> In this case, split the instruction that does not fit in a single
> controller-operation into two or more chunks.
> "
Ok.
>
> > + *
> > + * Returns true if the array of instruction must be split, false
> > otherwise.
>
> s/array of//
Right.
>
> > + * The @start_offset parameter is also updated to the offset at
> > which the first
> > + * instruction of the next bundle of instruction must start (if an
> > address or a
>
> "
> The @start_offset parameter is also updated to the offset at which the
> the next bundle of instructions must start ...
> "
Ok.
>
> > + * data instruction).
> > + */
> > +static bool
> > +nand_op_parser_must_split_instr(const struct
> > nand_op_parser_pattern_elem *pat,
> > + const struct nand_op_instr *instr,
> > + unsigned int *start_offset)
> > +{
[...]
> > +#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> > +static void nand_op_parser_trace(const struct nand_op_parser_ctx
> > *ctx,
> > + bool success)
> > +{
> > + const struct nand_op_instr *instr;
> > + bool in_subop = false;
> > + char *is_in = " ->", *is_out = " ", *prefix;
> > + char *buf;
> > + unsigned int len, off = 0;
> > + int i, j;
> > +
> > + if (success)
> > + pr_debug("executing subop:\n");
> > + else
> > + pr_debug("pattern not found:\n");
> > +
> > + for (i = 0; i < ctx->ninstrs; i++) {
> > + instr = &ctx->instrs[i];
> > +
> > + /*
> > + * ctx->instr_idx is not reliable because it may
> > already had
>
> have
Corrected.
>
> > + * been updated by the parser. Use pointers
> > comparison instead.
> > + */
> > + if (instr == &ctx->subop.instrs[0])
> > + in_subop = true;
>
> It seems in_subop is only used to select the prefix, so you can just
> get rid of it and do
>
> if (instr == &ctx->subop.instrs[0])
> prefix = " ->";
Ok, changed.
>
> BTW, if success is false, you should not even try to change the
> prefix, right?
I do not agree, there is no point in not showing what bundle of
instructions failed!
>
> > +
> > + if (in_subop)
> > + prefix = is_in;
> > + else
> > + prefix = is_out;
> > +
> > + switch (instr->type) {
> > + case NAND_OP_CMD_INSTR:
> > + pr_debug("%sCMD [0x%02x]\n", prefix,
> > + instr->cmd.opcode);
> > + break;
> > + case NAND_OP_ADDR_INSTR:
> > + /*
> > + * A log line is much less than 50 bytes,
> > plus 5 bytes
> > + * per address cycle to display.
> > + */
> > + len = 50 + 5 * instr->addr.naddrs;
> > + buf = kmalloc(len, GFP_KERNEL);
> > + if (!buf)
> > + return;
> > + memset(buf, 0, len);
> > + off += snprintf(buf, len, "ADDR [%d
> > cyc:",
> > + instr->addr.naddrs);
> > + for (j = 0; j < instr->addr.naddrs; j++)
> > + off += snprintf(&buf[off], len -
> > off, " 0x%02x",
> > +
> > instr->addr.addrs[j]);
> > + pr_debug("%s%s]\n", prefix, buf);
> > + break;
> > + case NAND_OP_DATA_IN_INSTR:
> > + pr_debug("%sDATA_IN [%d B%s]\n", prefix,
> > + instr->data.len,
> > + instr->data.force_8bit ? ", force
> > 8-bit" : "");
> > + break;
> > + case NAND_OP_DATA_OUT_INSTR:
> > + pr_debug("%sDATA_OUT [%d B%s]\n", prefix,
> > + instr->data.len,
> > + instr->data.force_8bit ? ", force
> > 8-bit" : "");
> > + break;
> > + case NAND_OP_WAITRDY_INSTR:
> > + pr_debug("%sWAITRDY [max %d ms]\n",
> > prefix,
> > + instr->waitrdy.timeout_ms);
> > + break;
> > + }
> > +
> > + if (instr == &ctx->subop.instrs[ctx->subop.ninstrs
> > - 1])
> > + in_subop = false;
>
> if (instr == &ctx->subop.instrs[ctx->subop.ninstrs -
> 1]) prefix = " ";
>
> and initialize prefix to " " at the beginning of the function.
>
> > + }
> > +}
> > +#else
> > +static void nand_op_parser_trace(const struct nand_op_parser_ctx
> > *ctx,
> > + bool success)
> > +{
> > + /* NOP */
> > +}
> > +#endif
> > +
> > +/**
> > + * nand_op_parser_exec_op - exec_op parser
> > + * @chip: the NAND chip
> > + * @parser: the parser to use given by the controller driver
> > + * @op: the NAND operation to address
> > + * @check_only: flag asking if the entire operation could be
> > handled
> > + *
> > + * Function that must be called by each driver that implement the
> > "exec_op API"
> > + * in their own ->exec_op() implementation.
> > + *
> > + * The function iterates on all the instructions asked and make
> > use of internal
> > + * parsers to find matches between the instruction list and the
> > handled patterns
> > + * filled by the controller drivers inside the @parser structure.
> > If needed, the
> > + * instructions could be split into sub-operations and be executed
> > sequentially.
> > + */
> > +int nand_op_parser_exec_op(struct nand_chip *chip,
> > + const struct nand_op_parser *parser,
> > + const struct nand_operation *op, bool
> > check_only) +{
> > + struct nand_op_parser_ctx ctx = {
> > + .instrs = op->instrs,
> > + .ninstrs = op->ninstrs,
> > + };
> > + unsigned int i;
> > +
> > + while (ctx.instr_idx < op->ninstrs) {
> > + bool pattern_found = false;
> > + int ret;
> > +
> > + for (i = 0; i < parser->npatterns; i++) {
> > + const struct nand_op_parser_pattern
> > *pattern; +
> > + pattern = &parser->patterns[i];
> > + if (!nand_op_parser_match_pat(pattern,
> > &ctx))
> > + continue;
> > +
> > + nand_op_parser_trace(&ctx, true);
> > +
>
> pattern_found should be set to true here. But I'm not even sure you
> really need this variable.
>
> > + if (check_only)
> > + break;
> > +
> > + ret = pattern->exec(chip, &ctx.subop);
> > + if (ret)
> > + return ret;
> > +
> > + pattern_found = true;
> > + }
> > +
> > + if (!pattern_found) {
>
> Just test
>
> if (i == parser->npatterns) {
>
> and you should be good.
Indeed it works.
>
> > + nand_op_parser_trace(&ctx, false);
> > + return -ENOTSUPP;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(nand_op_parser_exec_op);
> > +
> > +static bool nand_instr_is_data(const struct nand_op_instr *instr)
> > +{
> > + return instr && (instr->type == NAND_OP_DATA_IN_INSTR ||
> > + instr->type == NAND_OP_DATA_OUT_INSTR);
> > +}
> > +
> > +static bool nand_subop_instr_is_valid(const struct nand_subop
> > *subop,
> > + unsigned int op_id)
>
> s/op_id/instr_idx/
Changed everywhere.
>
> > +{
> > + return subop && op_id < subop->ninstrs;
> > +}
> > +
> > +static int nand_subop_get_start_off(const struct nand_subop *subop,
> > + unsigned int op_id)
>
> Ditto.
>
> > +{
> > + if (op_id)
> > + return 0;
> > +
> > + return subop->first_instr_start_off;
> > +}
> > +
> > +/**
> > + * nand_subop_get_addr_start_off - Get the start offset in an
> > address array
> > + * @subop: The entire sub-operation
> > + * @op_id: Index of the instruction inside the sub-operation
>
> instr_idx.
>
> > + *
> > + * Instructions arrays may be split by the parser between
> > instructions,
> > + * and also in the middle of an address instruction if the number
> > of cycles
> > + * to assert in one operation is not supported by the controller.
> > + *
> > + * For this, instead of using the first index of the ->addr.addrs
> > field from the
> > + * address instruction, the NAND controller driver must use this
> > helper that
> > + * will either return 0 if the index does not point to the first
> > instruction of
> > + * the sub-operation, or the offset of the next starting offset
> > inside the
> > + * address cycles.
>
> I think you can drop this paragraph which IMO brings more confusion to
> what this function does.
>
> > + *
> > + * Returns the offset of the first address cycle to assert from
> > the pointed
> > + * address instruction.
>
> This part is definitely clearer.
Ok.
>
> > + */
> > +int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> > + unsigned int op_id)
> > +{
> > + if (!nand_subop_instr_is_valid(subop, op_id) ||
> > + subop->instrs[op_id].type != NAND_OP_ADDR_INSTR)
> > + return -EINVAL;
> > +
> > + return nand_subop_get_start_off(subop, op_id);
> > +}
> > +EXPORT_SYMBOL_GPL(nand_subop_get_addr_start_off);
> > +
> > +/**
> > + * nand_subop_get_num_addr_cyc - Get the remaining address cycles
> > to assert
> > + * @subop: The entire sub-operation
> > + * @op_id: Index of the instruction inside the sub-operation
>
> instr_idx
>
> > + *
> > + * Instructions arrays may be split by the parser between
> > instructions,
> > + * and also in the middle of an address instruction if the number
> > of cycles
> > + * to assert in one operation is not supported by the controller.
> > + *
> > + * For this, instead of using the ->addr.naddrs fields of the
> > address
> > + * instruction, the NAND controller driver must use this helper
> > that will
> > + * return the actual number of cycles to assert between the first
> > and last
> > + * offset asked for this particular instruction.
> > + *
> > + * Returns the number of address cycles to assert from the pointed
> > address
> > + * instruction.
> > + */
> > +int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> > + unsigned int op_id)
> > +{
> > + int start_off, end_off;
> > +
> > + if (!nand_subop_instr_is_valid(subop, op_id) ||
> > + subop->instrs[op_id].type != NAND_OP_ADDR_INSTR)
> > + return -EINVAL;
> > +
> > + start_off = nand_subop_get_addr_start_off(subop, op_id);
> > +
> > + if (op_id == subop->ninstrs - 1 &&
> > + subop->last_instr_end_off)
> > + end_off = subop->last_instr_end_off;
> > + else
> > + end_off = subop->instrs[op_id].addr.naddrs;
> > +
> > + return end_off - start_off;
> > +}
> > +EXPORT_SYMBOL_GPL(nand_subop_get_num_addr_cyc);
> > +
> > +/**
> > + * nand_subop_get_data_start_off - Get the start offset in a data
> > array
> > + * @subop: The entire sub-operation
> > + * @op_id: Index of the instruction inside the sub-operation
> > + *
> > + * Instructions arrays may be split by the parser between
> > instructions,
> > + * and also in the middle of a data instruction if the number of
> > bytes to access
> > + * in one operation is greater that the controller limit.
> > + *
> > + * For this, instead of using the first index of the ->data.buf
> > field from the
> > + * data instruction, the NAND controller driver must use this
> > helper that
> > + * will either return 0 if the index does not point to the first
> > instruction of
> > + * the sub-operation, or the offset of the next starting offset
> > inside the
> > + * data buffer.
>
> Same as for nand_subop_get_addr_start_off(), the explanation is
> confusing. I think we can drop it.
Dropped then.
>
> > + *
> > + * Returns the data offset inside the pointed data instruction
> > buffer from which
> > + * to start.
> > + */
> > +int nand_subop_get_data_start_off(const struct nand_subop *subop,
> > + unsigned int op_id)
> > +{
> > + if (!nand_subop_instr_is_valid(subop, op_id) ||
> > + !nand_instr_is_data(&subop->instrs[op_id]))
> > + return -EINVAL;
> > +
> > + return nand_subop_get_start_off(subop, op_id);
> > +}
> > +EXPORT_SYMBOL_GPL(nand_subop_get_data_start_off);
> > +
> > +/**
> > + * nand_subop_get_data_len - Get the number of bytes to retrieve
> > + * @subop: The entire sub-operation
> > + * @op_id: Index of the instruction inside the sub-operation
> > + *
> > + * Instructions arrays may be split by the parser between
> > instructions,
> > + * and also in the middle of a data instruction if the number of
> > bytes to access
> > + * in one operation is greater that the controller limit.
> > + *
> > + * For this, instead of using the ->data.len field from the data
> > instruction,
> > + * the NAND controller driver must use this helper that will
> > return the actual
> > + * length of data to move between the first and last offset asked
> > for this
> > + * particular instruction.
> > + *
> > + * Returns the length of the data to move from the pointed data
> > instruction.
> > + */
> > +int nand_subop_get_data_len(const struct nand_subop *subop,
> > + unsigned int op_id)
> > +{
> > + int start_off = 0, end_off;
> > +
> > + if (!nand_subop_instr_is_valid(subop, op_id) ||
> > + !nand_instr_is_data(&subop->instrs[op_id]))
> > + return -EINVAL;
> > +
> > + start_off = nand_subop_get_data_start_off(subop, op_id);
> > +
> > + if (op_id == subop->ninstrs - 1 &&
> > + subop->last_instr_end_off)
> > + end_off = subop->last_instr_end_off;
> > + else
> > + end_off = subop->instrs[op_id].data.len;
> > +
> > + return end_off - start_off;
> > +}
> > +EXPORT_SYMBOL_GPL(nand_subop_get_data_len);
> > +
> > +/**
> > * nand_reset - Reset and initialize a NAND device
> > * @chip: The NAND chip
> > * @chipnr: Internal die id
> > @@ -3940,7 +4815,7 @@ static void nand_set_defaults(struct
> > nand_chip *chip) chip->chip_delay = 20;
> >
> > /* check, if a user supplied command function given */
> > - if (chip->cmdfunc == NULL)
> > + if (chip->cmdfunc == NULL && !chip->exec_op)
> > chip->cmdfunc = nand_command;
> >
> > /* check, if a user supplied wait function given */
> > @@ -4823,15 +5698,35 @@ int nand_scan_ident(struct mtd_info *mtd,
> > int maxchips, if (!mtd->name && mtd->dev.parent)
> > mtd->name = dev_name(mtd->dev.parent);
> >
> > - if ((!chip->cmdfunc || !chip->select_chip)
> > && !chip->cmd_ctrl) {
> > + /*
> > + * ->cmdfunc() is legacy and will only be used if
> > ->exec_op() is not
> > + * populated.
> > + */
> > + if (chip->exec_op) {
> > /*
> > - * Default functions assigned for chip_select() and
> > - * cmdfunc() both expect cmd_ctrl() to be
> > populated,
> > - * so we need to check that that's the case
> > + * The implementation of ->exec_op() heavily
> > relies on timings
> > + * to be accessible through the
> > nand_data_interface structure.
> > + * Thus, the ->setup_data_interface() hook must be
> > provided. The
> > + * controller driver will be noticed of delays it
> > must apply
> > + * after each ->exec_op() instruction (if any)
> > through the
> > + * .delay_ns field. The driver will then choose to
> > handle the
> > + * delays manually if the controller cannot do it
> > natively. */
> > - pr_err("chip.cmd_ctrl() callback is not provided");
> > - return -EINVAL;
> > + if (!chip->setup_data_interface) {
> > + pr_err("->setup_data_interface() should be
> > provided\n");
> > + return -EINVAL;
> > + }
> > + } else {
> > + /*
> > + * Default functions assigned for ->cmdfunc() and
> > + * ->select_chip() both expect ->cmd_ctrl() to be
> > populated.
> > + */
> > + if ((!chip->cmdfunc || !chip->select_chip)
> > && !chip->cmd_ctrl) {
> > + pr_err("->cmd_ctrl() should be
> > provided\n");
> > + return -EINVAL;
> > + }
> > }
> > +
> > /* Set the default functions */
> > nand_set_defaults(chip);
> >
> > diff --git a/drivers/mtd/nand/nand_hynix.c
> > b/drivers/mtd/nand/nand_hynix.c index 04e3ab7a476c..81c382f24513
> > 100644 --- a/drivers/mtd/nand/nand_hynix.c
> > +++ b/drivers/mtd/nand/nand_hynix.c
> > @@ -74,19 +74,36 @@ static bool hynix_nand_has_valid_jedecid(struct
> > nand_chip *chip) return !strncmp("JEDEC", jedecid, sizeof(jedecid));
> > }
> >
> > +static int hynix_nand_cmd_op(struct nand_chip *chip, u8 cmd)
>
> Maybe you can introduce the helper in patch 1
Done.
>
> > +{
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > + if (chip->exec_op) {
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(cmd, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + return nand_exec_op(chip, &op);
>
> And then ass this block of code in this patch.
Sure.
>
> > + }
> > +
> > + chip->cmdfunc(mtd, cmd, -1, -1);
> > +
> > + return 0;
> > +}
> > +
> > static int hynix_nand_setup_read_retry(struct mtd_info *mtd, int
> > retry_mode) {
> > struct nand_chip *chip = mtd_to_nand(mtd);
> > struct hynix_nand *hynix =
> > nand_get_manufacturer_data(chip); const u8 *values;
> > - int status;
> > int i;
> >
> > values = hynix->read_retry->values +
> > (retry_mode * hynix->read_retry->nregs);
> >
> > /* Enter 'Set Hynix Parameters' mode */
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_SET_PARAMS, -1, -1);
> > + hynix_nand_cmd_op(chip, NAND_HYNIX_CMD_SET_PARAMS);
> >
> > /*
> > * Configure the NAND in the requested read-retry mode.
> > @@ -101,16 +118,17 @@ static int hynix_nand_setup_read_retry(struct
> > mtd_info *mtd, int retry_mode) int column =
> > hynix->read_retry->regs[i];
> > column |= column << 8;
> > - chip->cmdfunc(mtd, NAND_CMD_NONE, column, -1);
> > - chip->write_byte(mtd, values[i]);
> > + if (chip->exec_op) {
> > + nand_change_write_column_op(chip, column,
> > + &values[i], 1,
> > true);
>
> This is not a nand_change_write_column_op() op, here the cmd cycle is
> set to NAND_CMD_NONE. You should have your own operation defined to
> handle this sequence.
That is right.
However, this is not handled correctly in nand_command_lp(), I will
send a patch very soon to fix it.
>
> > + } else {
> > + chip->cmdfunc(mtd, NAND_CMD_NONE, column,
> > -1);
> > + chip->write_byte(mtd, values[i]);
> > + }
> > }
> >
> > /* Apply the new settings. */
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_APPLY_PARAMS, -1, -1);
> > -
> > - status = chip->waitfunc(mtd, chip);
> > - if (status & NAND_STATUS_FAIL)
> > - return -EIO;
> > + hynix_nand_cmd_op(chip, NAND_HYNIX_CMD_APPLY_PARAMS);
>
> No, it's wrong, you miss the WAITRDY instruction to be compatible
> with what was done before.
Re-added.
>
> >
> > return 0;
> > }
> > @@ -173,32 +191,65 @@ static int hynix_read_rr_otp(struct nand_chip
> > *chip,
> > nand_reset_op(chip);
> >
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_SET_PARAMS, -1, -1);
> > + hynix_nand_cmd_op(chip, NAND_HYNIX_CMD_SET_PARAMS);
> >
> > for (i = 0; i < info->nregs; i++) {
> > int column = info->regs[i];
> >
> > column |= column << 8;
> > - chip->cmdfunc(mtd, NAND_CMD_NONE, column, -1);
> > - chip->write_byte(mtd, info->values[i]);
> > + if (chip->exec_op) {
> > + nand_change_write_column_op(chip, column,
> > +
> > &info->values[i], 1, true);
> > + } else {
> > + chip->cmdfunc(mtd, NAND_CMD_NONE, column,
> > -1);
> > + chip->write_byte(mtd, info->values[i]);
> > + }
>
> Same comments apply here.
>
> > }
> >
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_APPLY_PARAMS, -1, -1);
> > + hynix_nand_cmd_op(chip, NAND_HYNIX_CMD_APPLY_PARAMS);
> >
> > /* Sequence to enter OTP mode? */
> > - chip->cmdfunc(mtd, 0x17, -1, -1);
> > - chip->cmdfunc(mtd, 0x04, -1, -1);
> > - chip->cmdfunc(mtd, 0x19, -1, -1);
> > + if (chip->exec_op) {
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(0x17, 0),
> > + NAND_OP_CMD(0x04, 0),
> > + NAND_OP_CMD(0x19, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + nand_exec_op(chip, &op);
> > + } else {
> > + chip->cmdfunc(mtd, 0x17, -1, -1);
> > + chip->cmdfunc(mtd, 0x04, -1, -1);
> > + chip->cmdfunc(mtd, 0x19, -1, -1);
> > + }
>
> Why not creating a hynix_nand_enter_otp_mode_op() for that?
I think this file needs some kind of rework and may really benefit
from ->exec_op() new API, but this series is maybe not the best place to
do it. Let's keep it this way for now.
>
> >
> > /* Now read the page */
> > nand_read_page_op(chip, info->page, 0, buf, info->size);
> >
> > /* Put everything back to normal */
> > nand_reset_op(chip);
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_SET_PARAMS, 0x38, -1);
> > - chip->write_byte(mtd, 0x0);
> > - chip->cmdfunc(mtd, NAND_HYNIX_CMD_APPLY_PARAMS, -1, -1);
> > - chip->cmdfunc(mtd, NAND_CMD_READ0, 0x0, -1);
> > + if (chip->exec_op) {
> > + const struct nand_sdr_timings *sdr =
> > +
> > nand_get_sdr_timings(&chip->data_interface);
> > + u8 byte = 0;
> > + u8 addr = 0x38;
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_HYNIX_CMD_SET_PARAMS, 0),
> > + NAND_OP_ADDR(1, &addr, sdr->tCCS_min),
> > + NAND_OP_8BIT_DATA_OUT(1, &byte, 0),
> > + NAND_OP_CMD(NAND_HYNIX_CMD_APPLY_PARAMS,
> > 0),
> > + NAND_OP_CMD(NAND_CMD_READ0, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + nand_exec_op(chip, &op);
> > + } else {
> > + chip->cmdfunc(mtd, NAND_HYNIX_CMD_SET_PARAMS,
> > 0x38, -1);
> > + chip->write_byte(mtd, 0x0);
> > + chip->cmdfunc(mtd, NAND_HYNIX_CMD_APPLY_PARAMS,
> > -1, -1);
> > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x0, -1);
> > + }
>
> Same here.
>
> >
> > return 0;
> > }
> > diff --git a/drivers/mtd/nand/nand_micron.c
> > b/drivers/mtd/nand/nand_micron.c index 543352380ffa..109d8003e33d
> > 100644 --- a/drivers/mtd/nand/nand_micron.c
> > +++ b/drivers/mtd/nand/nand_micron.c
> > @@ -117,14 +117,38 @@ micron_nand_read_page_on_die_ecc(struct
> > mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int
> > oob_required, int page)
> > {
> > - int status;
> > + u8 status;
> > int max_bitflips = 0;
> >
> > micron_nand_on_die_ecc_setup(chip, true);
> >
> > - chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> > - chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> > - status = chip->read_byte(mtd);
> > + if (chip->exec_op) {
> > + u8 addrs[5] = {};
> > + struct nand_op_instr instrs[] = {
> > + NAND_OP_CMD(NAND_CMD_READ0, 0),
> > + NAND_OP_ADDR(4, addrs, 0),
> > + NAND_OP_CMD(NAND_CMD_STATUS, 0),
> > + NAND_OP_8BIT_DATA_IN(1, &status, 0),
> > + };
> > + struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > + if (nand_fill_column_cycles(chip, addrs, 0))
>
> if (nand_fill_column_cycles(chip, addrs, 0) < 0)
Yes.
>
> > + return -EINVAL;
> > +
> > + addrs[2] = page;
> > + addrs[3] = page >> 8;
> > + if (chip->options & NAND_ROW_ADDR_3) {
> > + addrs[4] = page >> 16;
> > + instrs[1].addr.naddrs++;
> > + }
> > +
> > + nand_exec_op(chip, &op);
> > + } else {
> > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> > + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> > + status = chip->read_byte(mtd);
> > + }
> > +
> > if (status & NAND_STATUS_FAIL)
> > mtd->ecc_stats.failed++;
> > /*
> > diff --git a/include/linux/mtd/rawnand.h
> > b/include/linux/mtd/rawnand.h index 1acc26ed0e91..302f231df65e
> > 100644 --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -751,6 +751,337 @@ struct nand_manufacturer_ops {
> > };
> >
> > /**
> > + * struct nand_op_cmd_instr - Definition of a command instruction
> > + * @opcode: the command to assert in one cycle
> > + */
> > +struct nand_op_cmd_instr {
> > + u8 opcode;
> > +};
> > +
> > +/**
> > + * struct nand_op_addr_instr - Definition of an address instruction
> > + * @naddrs: length of the @addrs array
> > + * @addrs: array containing the address cycles to assert
> > + */
> > +struct nand_op_addr_instr {
> > + unsigned int naddrs;
> > + const u8 *addrs;
> > +};
> > +
> > +/**
> > + * struct nand_op_data_instr - Definition of a data instruction
> > + * @len: number of data bytes to move
> > + * @in: buffer to fill when reading from the NAND chip
> > + * @out: buffer to read from when writing to the NAND chip
> > + * @force_8bit: force 8-bit access
> > + *
> > + * Please note that "in" and "out" are inverted from the ONFI
> > specification
> > + * and are from the controller perspective, so a "in" is a read
> > from the NAND
> > + * chip while a "out" is a write to the NAND chip.
> > + */
> > +struct nand_op_data_instr {
> > + unsigned int len;
> > + union {
> > + void *in;
> > + const void *out;
> > + };
> > + bool force_8bit;
> > +};
> > +
> > +/**
> > + * struct nand_op_waitrdy_instr - Definition of a wait ready
> > instruction
> > + * @timeout_ms: maximum delay while waiting for the ready/busy pin
> > in ms
> > + */
> > +struct nand_op_waitrdy_instr {
> > + unsigned int timeout_ms;
> > +};
> > +
> > +/**
> > + * enum nand_op_instr_type - Enumeration of all the instructions
>
> *of all instruction types
Changed.
>
> > + *
> > + * Please note that data instructions are separated into DATA_IN
> > and DATA_OUT
> > + * instructions.
> > + */
> > +enum nand_op_instr_type {
> > + NAND_OP_CMD_INSTR,
> > + NAND_OP_ADDR_INSTR,
> > + NAND_OP_DATA_IN_INSTR,
> > + NAND_OP_DATA_OUT_INSTR,
> > + NAND_OP_WAITRDY_INSTR,
> > +};
> > +
> > +/**
> > + * struct nand_op_instr - Generic definition of and instruction
>
> s/and/an/
Corrected.
>
> > + * @type: an enumeration of the instruction type
> > + * @cmd/@addr/@data/@waitrdy: the actual instruction to refer
> > depending on the
> > + * value of @type
>
> "
> extra data associated to the instruction. You'll have to use
> the appropriate element depending on @type"
Changed.
>
> > + * @delay_ns: delay to apply by the controller after the
> > instruction has been
> > + * actually executed (most of them are directly
> > handled by the
> > + * controllers once the timings negociation has been
> > done)
> > + */
> > +struct nand_op_instr {
> > + enum nand_op_instr_type type;
> > + union {
> > + struct nand_op_cmd_instr cmd;
> > + struct nand_op_addr_instr addr;
> > + struct nand_op_data_instr data;
> > + struct nand_op_waitrdy_instr waitrdy;
> > + };
> > + unsigned int delay_ns;
> > +};
> > +
> > +/*
> > + * Special handling must be done for the WAITRDY timeout parameter
> > as it usually
> > + * is either tPROG (after a prog), tR (before a read), tRST
> > (during a reset) or
> > + * tBERS (during an erase) which all of them are u64 values that
> > cannot be
> > + * divided by usual kernel macros and must be handled with the
> > special
> > + * DIV_ROUND_UP_ULL() macro.
> > + */
> > +#define PSEC_TO_NSEC(x) DIV_ROUND_UP(x, 10^3)
> > +#define PSEC_TO_MSEC(x) DIV_ROUND_UP_ULL(x, 10^9)
>
> Hm, that's a bit of a mess to let each macro decide which converter
> they should use, because we don't know at this level whether the
> timing is stored in an u64 or u32. How about doing the conversion in
> the call-sites instead, because there you should know what you
> manipulate.
>
> Something like
>
> NAND_OP_CMD(FOO, PSEC_TO_NSEC_UL(sdr->tXX))
> NAND_OP_WAITRDY(PSEC_TO_MSEC_ULL(sdr->tXX),
> PSEC_TO_NSEC_UL(sdr->tYY))
That is a bit messy but okay. I have added to the core a "__DIVIDE"
macro that uses the size of the dividend to decide if the *_ULL version
must be used, so the drivers can safely use PSEC_TO_NSEC and
PSEC_TO_MSEC without knowing it the timings are stored in a u32 or a
u64.
>
>
>
> > +
> > +#define NAND_OP_CMD(id,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_CMD_INSTR, \
> > + .cmd.opcode =
> > id, \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_ADDR(ncycles, cycles,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_ADDR_INSTR, \
> > + .addr =
> > { \
> > + .naddrs =
> > ncycles, \
> > + .addrs =
> > cycles, \
> > + },
> > \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_DATA_IN(l, buf,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_DATA_IN_INSTR, \
> > + .data =
> > { \
> > + .len =
> > l, \
> > + .in =
> > buf, \
> > + .force_8bit =
> > false, \
> > + },
> > \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_DATA_OUT(l, buf,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_DATA_OUT_INSTR, \
> > + .data =
> > { \
> > + .len =
> > l, \
> > + .out =
> > buf, \
> > + .force_8bit =
> > false, \
> > + },
> > \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_8BIT_DATA_IN(l, buf,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_DATA_IN_INSTR, \
> > + .data =
> > { \
> > + .len =
> > l, \
> > + .in =
> > buf, \
> > + .force_8bit =
> > true, \
> > + },
> > \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_8BIT_DATA_OUT(l, buf,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_DATA_OUT_INSTR, \
> > + .data =
> > { \
> > + .len =
> > l, \
> > + .out =
> > buf, \
> > + .force_8bit =
> > true, \
> > + },
> > \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +#define NAND_OP_WAIT_RDY(tout_ps,
> > delay_ps) \
> > +
> > { \
> > + .type =
> > NAND_OP_WAITRDY_INSTR, \
> > + .waitrdy.timeout_ms =
> > PSEC_TO_MSEC(tout_ps), \
> > + .delay_ns =
> > PSEC_TO_NSEC(delay_ps), \
> > + }
> > +
> > +/**
> > + * struct nand_subop - a sub operation
> > + * @instrs: array of instructions
> > + * @ninstrs: length of the @instrs array
> > + * @first_instr_start_off: offset to start from for the first
> > instruction
> > + * of the sub-operation
> > + * @last_instr_end_off: offset to end at (excluded) for the last
> > instruction
> > + * of the sub-operation
> > + *
> > + * When an operation cannot be handled as is by the NAND
> > controller, it will
> > + * be split by the parser and the remaining pieces will be handled
> > as
> > + * sub-operations.
> > + */
> > +struct nand_subop {
> > + const struct nand_op_instr *instrs;
> > + unsigned int ninstrs;
> > + /*
> > + * Specific offset for the first and the last instructions
> > of the subop.
> > + * Applies for the address cycles in the case of address,
> > or for data
> > + * offset in the case of data transfers. Otherwise, it is
> > irrelevant.
> > + */
>
> Can you move that in the kernel header doc?
Sure.
>
> > + unsigned int first_instr_start_off;
> > + unsigned int last_instr_end_off;
> > +};
> > +
> > +int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> > + unsigned int op_id);
> > +int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> > + unsigned int op_id);
> > +int nand_subop_get_data_start_off(const struct nand_subop *subop,
> > + unsigned int op_id);
> > +int nand_subop_get_data_len(const struct nand_subop *subop,
> > + unsigned int op_id);
> > +
> > +/**
> > + * struct nand_op_parser_addr_constraints - Constraints for
> > address instructions
> > + * @maxcycles: maximum number of cycles that the controller can
> > assert by
> > + * instruction
> > + */
> > +struct nand_op_parser_addr_constraints {
> > + unsigned int maxcycles;
> > +};
> > +
> > +/**
> > + * struct nand_op_parser_data_constraints - Constraints for data
> > instructions
> > + * @maxlen: maximum data length that the controller can handle
> > with one
> > + * instruction
> > + */
> > +struct nand_op_parser_data_constraints {
> > + unsigned int maxlen;
>
> At some point we should probably add minlen and alignment fields, but
> let's keep that for later.
>
> > +};
> > +
> > +/**
> > + * struct nand_op_parser_pattern_elem - One element of a pattern
> > + * @type: the instructuction type
> > + * @optional: if this element of the pattern is optional or
> > mandatory
> > + * @addr/@data: address or data constraint (number of cycles or
> > data length)
> > + */
> > +struct nand_op_parser_pattern_elem {
> > + enum nand_op_instr_type type;
> > + bool optional;
> > + union {
> > + struct nand_op_parser_addr_constraints addr;
> > + struct nand_op_parser_data_constraints data;
> > + };
> > +};
> > +
> > +#define NAND_OP_PARSER_PAT_CMD_ELEM(_opt) \
> > + { \
> > + .type = NAND_OP_CMD_INSTR, \
> > + .optional = _opt, \
> > + }
> > +
> > +#define NAND_OP_PARSER_PAT_ADDR_ELEM(_opt,
> > _maxcycles) \
> > + { \
> > + .type = NAND_OP_ADDR_INSTR,
> > \
> > + .optional = _opt, \
> > + .addr.maxcycles =
> > _maxcycles, \
> > + }
> > +
> > +#define NAND_OP_PARSER_PAT_DATA_IN_ELEM(_opt,
> > _maxlen) \
> > + { \
> > + .type =
> > NAND_OP_DATA_IN_INSTR, \
> > + .optional = _opt, \
> > + .data.maxlen =
> > _maxlen, \
> > + }
> > +
> > +#define NAND_OP_PARSER_PAT_DATA_OUT_ELEM(_opt,
> > _maxlen) \
> > + { \
> > + .type =
> > NAND_OP_DATA_OUT_INSTR, \
> > + .optional = _opt, \
> > + .data.maxlen =
> > _maxlen, \
> > + }
> > +
> > +#define
> > NAND_OP_PARSER_PAT_WAITRDY_ELEM(_opt) \
> > + { \
> > + .type =
> > NAND_OP_WAITRDY_INSTR, \
> > + .optional = _opt, \
> > + }
> > +
> > +/**
> > + * struct nand_op_parser_pattern - A complete pattern
> > + * @elems: array of pattern elements
> > + * @nelems: number of pattern elements in @elems array
> > + * @exec: the function that will actually execute this pattern,
> > written in the
> > + * controller driver
> > + *
> > + * This is a complete pattern that is a list of elements, each one
> > reprensenting
> > + * one instruction with its constraints. Controller drivers must
> > declare as much
> > + * patterns as they support and give the list of the supported
> > patterns (created
> > + * with the help of the following macro) at the call to
> > nand_op_parser_exec_op
>
> s/at the call to nand_op_parser_exec_op/when calling
> nand_op_parser_exec_op()/
Ok.
>
> > + * which shall be the main thing to do in the driver
> > implementation of
> > + * ->exec_op().
>
> I'd be more lax than that. Yes the parser is the preferred approach
> when you deal with a complex controller. But for simple controllers
> it's not.
>
> > Once there is a match between the pattern and an operation, the
> > + * core calls the @exec function to actually do the operation.
>
> Not necessarily. The plan is still to ask the controller which
> operation it supports before actually executing them. So, in this case
> (when check_only param is true), nand_op_parser_exec_op() will never
> call ->exec(), it will just make sure the operation can be handled
> with the provided patterns.
I changed that §, see next version.
>
> > + */
> > +struct nand_op_parser_pattern {
> > + const struct nand_op_parser_pattern_elem *elems;
> > + unsigned int nelems;
> > + int (*exec)(struct nand_chip *chip, const struct
> > nand_subop *subop); +};
> > +
> > +#define
> > NAND_OP_PARSER_PATTERN(_exec, ...)
> > \
> > +
> > {
> > \
> > + .exec =
> > _exec,
> > \
> > + .elems = (struct nand_op_parser_pattern_elem[])
> > { __VA_ARGS__ }, \
> > + .nelems = sizeof((struct
> > nand_op_parser_pattern_elem[]) { __VA_ARGS__ }) / \
> > + sizeof(struct
> > nand_op_parser_pattern_elem), \
> > + }
> > +
> > +/**
> > + * struct nand_op_parser - The actual parser
> > + * @patterns: array of patterns
> > + * @npatterns: length of the @patterns array
> > + *
> > + * The actual parser structure wich is an array of supported
> > patterns.
>
> It's worth mentioning that patterns will be tested in their
> declaration order, and the first match will be taken, so it's
> important to oder patterns appropriately so that simple/inefficient
> patterns are placed at the end of the list. Usually, this is where
> you put single instruction patterns.
Nice explanation, added.
>
> > + */
> > +struct nand_op_parser {
> > + const struct nand_op_parser_pattern *patterns;
> > + unsigned int npatterns;
> > +};
> > +
> > +#define
> > NAND_OP_PARSER(...)
> > \
> > +
> > {
> > \
> > + .patterns = (struct nand_op_parser_pattern[])
> > { __VA_ARGS__ }, \
> > + .npatterns = sizeof((struct
> > nand_op_parser_pattern[]) { __VA_ARGS__ }) / \
> > + sizeof(struct
> > nand_op_parser_pattern), \
> > + }
> > +
> > +/**
> > + * struct nand_operation - The actual operation
> > + * @instrs: array of instructions to execute
> > + * @ninstrs: length of the @instrs array
> > + *
> > + * The actual operation structure that will be given to the
> > parser.
>
> Not only the parser, it will be passed to chip->exep_op() as well.
Yes.
>
> > + */
> > +struct nand_operation {
> > + const struct nand_op_instr *instrs;
> > + unsigned int ninstrs;
> > +};
> > +
> > +#define
> > NAND_OPERATION(_instrs) \
> > + { \
> > + .instrs = _instrs, \
> > + .ninstrs =
> > ARRAY_SIZE(_instrs), \
> > + }
> > +
> > +int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs,
> > + unsigned int offset_in_page);
> > +
> > +int nand_op_parser_exec_op(struct nand_chip *chip,
> > + const struct nand_op_parser *parser,
> > + const struct nand_operation *op, bool
> > check_only); +
> > +/**
> > * struct nand_chip - NAND Private Flash Chip Data
> > * @mtd: MTD device registered to the MTD framework
> > * @IO_ADDR_R: [BOARDSPECIFIC] address to read the
> > 8 I/O lines of the @@ -885,6 +1216,9 @@ struct nand_chip {
> > int (*setup_data_interface)(struct mtd_info *mtd, int
> > chipnr, const struct nand_data_interface *conf);
> >
> > + int (*exec_op)(struct nand_chip *chip,
> > + const struct nand_operation *op,
> > + bool check_only);
> >
> > int chip_delay;
> > unsigned int options;
> > @@ -945,6 +1279,15 @@ struct nand_chip {
> > } manufacturer;
> > };
> >
> > +static inline int nand_exec_op(struct nand_chip *chip,
> > + const struct nand_operation *op)
> > +{
> > + if (!chip->exec_op)
> > + return -ENOTSUPP;
> > +
> > + return chip->exec_op(chip, op, false);
> > +}
> > +
> > extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
> > extern const struct mtd_ooblayout_ops nand_ooblayout_lp_ops;
> >
> > @@ -1307,23 +1650,26 @@ int nand_readid_op(struct nand_chip *chip,
> > u8 addr, void *buf, int nand_status_op(struct nand_chip *chip, u8
> > *status); int nand_erase_op(struct nand_chip *chip, unsigned int
> > eraseblock); int nand_read_page_op(struct nand_chip *chip, unsigned
> > int page,
> > - unsigned int column, void *buf, unsigned int
> > len); -int nand_change_read_column_op(struct nand_chip *chip,
> > unsigned int column,
> > - void *buf, unsigned int len, bool
> > force_8bit);
> > + unsigned int offset_in_page, void *buf,
> > unsigned int len); +int nand_change_read_column_op(struct nand_chip
> > *chip,
> > + unsigned int offset_in_page, void
> > *buf,
> > + unsigned int len, bool force_8bit);
> > int nand_read_oob_op(struct nand_chip *chip, unsigned int page,
> > - unsigned int column, void *buf, unsigned int
> > len);
> > + unsigned int offset_in_page, void *buf,
> > unsigned int len); int nand_prog_page_begin_op(struct nand_chip
> > *chip, unsigned int page,
> > - unsigned int column, const void *buf,
> > + unsigned int offset_in_page, const
> > void *buf, unsigned int len);
> > int nand_prog_page_end_op(struct nand_chip *chip);
> > int nand_prog_page_op(struct nand_chip *chip, unsigned int page,
> > - unsigned int column, const void *buf,
> > unsigned int len); -int nand_change_write_column_op(struct
> > nand_chip *chip, unsigned int column,
> > - const void *buf, unsigned int len,
> > bool force_8bit);
> > + unsigned int offset_in_page, const void *buf,
> > + unsigned int len);
> > +int nand_change_write_column_op(struct nand_chip *chip,
> > + unsigned int offset_in_page, const
> > void *buf,
> > + unsigned int len, bool force_8bit);
> > int nand_read_data_op(struct nand_chip *chip, void *buf, unsigned
> > int len,
> > - bool force_8bits);
> > + bool force_8bit);
> > int nand_write_data_op(struct nand_chip *chip, const void *buf,
> > - unsigned int len, bool force_8bits);
> > + unsigned int len, bool force_8bit);
> >
> > /* Free resources held by the NAND device */
> > void nand_cleanup(struct nand_chip *chip);
>
Thanks for the detailed review,
Miquèl
More information about the linux-mtd
mailing list