Re: [PATCH v4 7/8] mtd: rawnand: arasan: Add new Arasan NAND controller

On Mon, 11 May 2020 17:07:29 +0200
Miquel Raynal [off-list ref] wrote:

Hi Boris,

Boris Brezillon [off-list ref] wrote on Sun, 10 May
2020 09:03:14 +0200:

quoted

On Fri,  8 May 2020 19:13:38 +0200
Miquel Raynal [off-list ref] wrote:
  

quoted

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