Re: [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Jan,

On Sun, 30 Apr 2017 15:01:00 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> 
> > 
> > How far are you with the new interface, can you share some code?  
> 
> I started to rework the NAND framework a while ago [1], but never had
> time to finish it. I think I was too ambitious, so let's try to be
> pragmatic this time, and focus on one problem at a time.
> 
> Your problem here is the separation of the CMD/ADDR cycles (done in
> ->cmdfunc() and/or cmd_ctrl()) and the DATA cycles (done in
> ->read/write_buf/byte/word()), which complexifies the driver logic.  
> 
> What you should look at is defining a proper nand_operation object
> (here's my initial definition [2], but you may want/need to remove some
> fields or add new ones) and add a new ->exec_op() hook to nand_chip
> taking a nand_operation struct (+ a pointer to the nand_chip, of
> course).
> 
> Once you have that, you should patch all accesses from the core to use
> the new ->exec_op() interface instead of ->cmdfunc() +
> ->read/write_xx(). Of course, that means providing a compatibility layer  
> for all drivers still implementing ->cmdfunc(), which is probably the
> trickiest part of the job.
> 
> You'll have to rework the nand_do_read/write_ops() functions to
> prevent the separation of the ->cmdfunc() call (done in
> nand_do_read/write_ops() function) from the data transfer (done in
> chip->ecc.read/write_page_xxx()).
> 
> I'll try to come up with an initial/ugly patch to show you the
> direction, and I'll let you cleanup/massage the implementation ;-).

I finally had time to finish the PoC I had in mind [1], unfortunately I
didn't have time to provide a reference implementation to show you how
it is supposed to work from a driver PoV.

In your driver, you should have something like the following pseudo
code:

static int cavium_nfc_exec_op(struct nand_chip *chip,
			      struct nand_op_instr *instrs,
			      unsigned int ninstrs)
{
	int i;

	/* queue ACQ BUS + chip enable operation. */
	for (i = 0; i < ninstrs; i++) {
		switch (instrs[i].type)
		case NAND_OP_CMD_INSTR:
			/* queue CLE instruction */
			break;
		case NAND_OP_ADDR_INSTR:
			/* queue ALE instruction */
			break;
		case NAND_OP_DATA_IN_INSTR:
		case NAND_OP_DATA_OUT_INSTR:
		case NAND_OP_8BIT_DATA_IN_INSTR:
		case NAND_OP_8BIT_DATA_OUT_INSTR:
			/* queue DATA instruction + prepare DMA */
			break;
		case NAND_OP_WAIT_RDY:
			/* queue WAIT RDY or WAIT STATUS RDY ins */
			break;
	}

	/* Issue FULL operation, deselect CS, release bus, ... */

	return 0; /* or error code. */
}

BTW, I had time to look more closely at the cavium engine, and I have a
few questions/comments (coming soon).

Regarding the ->exec_op() interface, it's probably not perfect, so
please try to review it and let me know if you think something important
is missing.

Regards,

Boris

[1]https://github.com/bbrezillon/linux/commits/nand/exec_op