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

Hi Boris,

Boris Brezillon [off-list ref] wrote on Mon, 11 May
2020 17:32:35 +0200:

quoted

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

quoted

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.  

Oh you really want to extend the core for that? I thought having a
"check_op" helper like this was enough, as it gives enough freedom to
the controller driver to return all the corner cases that are not very
generic. See below for more details.

Widespread enough to have at least 2 controllers needing that :P. Any
code sharing is good to take IMO.

quoted

quoted

+        */    

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)?).  

Precisely, for me the previous check is not a problem from the core
perspective (hence not deserving a FIXME) because the driver do not lie
at any moment. Conversely, the driver limitations of what is supported
and what is not is clear and accurate.

True, you can turn that into a TODO. That's still something I'd like to
see shared. That's not to say we have to handle all weird cases in the
core, we can simply add a ->check() hook to the pattern_elem so drivers
can have custom per-element checks.

However for this round_up() operation you are talking about, this is an
issue as we have currently no mean to say to the core that something
different than ordered was actually requested by the driver, so there
is lying involved and this deserves a FIXME.

quoted

	/*
	 * 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.
	 */  

I want to put this comment where the round_up takes place.

Sounds good.

quoted

quoted

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

For this one, I am not against a FIXME as this is something that might
be useful for other drivers too.

Maybe TODO more than FIXME, you're right.