On Mon, 11 May 2020 17:07:29 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> Hi Boris,
>
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Sun, 10 May
> 2020 09:03:14 +0200:
>
> > On Fri, 8 May 2020 19:13:38 +0200
> > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >
> > > +static int anfc_exec_op(struct nand_chip *chip,
> > > + const struct nand_operation *op,
> > > + bool check_only)
> > > +{
> > > + int ret;
> > > +
> > > + if (check_only)
> > > + return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> > > + check_only);
> >
> > You should also check the DATA_IN/OUT size here ^.
>
> Here is my proposal:
>
> ---8<---
>
> +static int anfc_check_op(struct nand_chip *chip,
> + const struct nand_operation *op)
> +{
> + int op_id;
> +
> + /*
> + * The controller abstracts all the NAND operations and do not support
> + * data only operations.
* FIXME: The nand_op_parser framework should be extended to
* support custom checks on DATA instructions.
> + */
You also didn't mention the fact that the number of data cycles should
be aligned on 4 bytes, and that the controller might read/write more
than requested when that's not the case. But maybe you have that
comment elsewhere in the code (where you do the round_up(4)?).
/*
* Number of DATA cycles must be aligned on 4, that means the
* controller might read/write more than requested This is
* harmless most of the time as extra DATA are discarded in
* the write path and read pointer adjusted in the read path.
* FIXME: The core should mark operations where reading/writing
* more is allowed so the exec_op() implementation can take
* the right decision when the alignment constraint is not met:
* adjust the number of DATA cycles when it's allowed, and
* reject the operation otherwise.
*/
> + for (op_id = 0; op_id < op->ninstrs; op_id++) {
> + instr = &op->instrs[op_id];
> +
> + switch (instr->type) {
> + case NAND_OP_ADDR_INSTR:
> + if (instr->ctx.addr.naddrs > ANFC_MAX_ADDR_CYC)
> + return -ENOTSUPP;
> + break;
> + case NAND_OP_DATA_IN_INSTR:
> + case NAND_OP_DATA_OUT_INSTR:
> + if (instr->ctx.data.len > ANFC_MAX_CHUNK_SIZE)
> + return -ENOTSUPP;
> + break;
> + default:
> + }
> + }
> +
> + /*
> + * The controller does not allow to proceed with a CMD+DATA_IN cycle
> + * manually on the bus by reading data from the data register. Instead,
> + * the controller abstract a status read operation with its own status
> + * register after ordering a read status operation. Hence, we cannot
> + * support any CMD+DATA_IN operation other than a READ STATUS.
* FIXME: The nand_op_parser() framework should be extended to
* describe fixed patterns instead of open-coding this check
* here.
> + */
> + if (op->ninstrs == 2 &&
> + op->instrs[0].type == NAND_OP_CMD_INSTR &&
> + op->instrs[0].ctx.cmd.opcode != NAND_CMD_STATUS &&
> + op->instrs[1].type == NAND_OP_DATA_IN_INSTR)
> + return -ENOTSUPP;
> +
> + return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> + check_only);
> +}
> +
> static int anfc_exec_op(struct nand_chip *chip,
> const struct nand_operation *op,
> bool check_only)
> @@ -774,8 +813,7 @@ static int anfc_exec_op(struct nand_chip *chip,
> int ret;
>
> if (check_only)
> - return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> - check_only);
> + return anfc_check_op(chip, op);
>
> ret = anfc_select_target(chip, op->cs);
> if (ret)
>
> --->8---
>
> What do you think?
