Re: [PATCH 13/17] mtd: rawnand: cafe: Add exec_op() support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
2020 10:20:23 +0200:

> Implementing exec_op() will help us get rid of the legacy interface and
> should make drivers much cleaner too.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/cafe_nand.c | 137 ++++++++++++++++++++++++++++++-
>  1 file changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> index edf65197604b..ada9c8b06a41 100644
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -21,7 +21,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> -#include <linux/io.h>
> +#include <linux/iopoll.h>
>  
>  #define CAFE_NAND_CTRL1				0x00
>  #define CAFE_NAND_CTRL1_HAS_CMD			BIT(31)
> @@ -774,9 +774,144 @@ static void cafe_nand_detach_chip(struct nand_chip *chip)
>  	dma_free_coherent(&cafe->pdev->dev, 2112, cafe->dmabuf, cafe->dmaaddr);
>  }
>  
> +static int cafe_nand_exec_subop(struct nand_chip *chip,
> +				const struct nand_subop *subop)
> +{
> +	struct cafe_priv *cafe = nand_get_controller_data(chip);
> +	u32 ctrl1 = 0, ctrl2 = cafe->ctl2, addr1 = 0, addr2 = 0;
> +	const struct nand_op_instr *data_instr = NULL;
> +	u32 status, wait = CAFE_NAND_IRQ_CMD_DONE;
> +	bool waitrdy = false;
> +	unsigned int i, j;
> +	int ret;
> +
> +	if (WARN_ON(subop->cs > 1))
> +		return -EINVAL;
> +
> +	cafe->datalen = 0;
> +	ctrl1 |= CAFE_FIELD_PREP(NAND_CTRL1, CE, subop->cs);
> +
> +	for (i = 0; i < subop->ninstrs; i++) {
> +		const struct nand_op_instr *instr = &subop->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			if (WARN_ON((ctrl1 & CAFE_NAND_CTRL1_HAS_CMD) &&
> +				    (ctrl2 & CAFE_NAND_CTRL2_HAS_CMD2)))
> +				return -EINVAL;

Same comment as in the previous drivers, just showing it to do not
foget.

> +
> +			if (!(ctrl1 & CAFE_NAND_CTRL1_HAS_CMD))
> +				ctrl1 |= CAFE_NAND_CTRL1_HAS_CMD |
> +					 CAFE_FIELD_PREP(NAND_CTRL1, CMD,
> +							 instr->ctx.cmd.opcode);
> +			else
> +				ctrl2 |= CAFE_NAND_CTRL2_HAS_CMD2 |
> +					 CAFE_FIELD_PREP(NAND_CTRL2, CMD2,
> +							 instr->ctx.cmd.opcode);
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			if (WARN_ON(instr->ctx.addr.naddrs > 5 ||
> +				    !instr->ctx.addr.naddrs))
> +				return -EINVAL;
> +
> +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> +				u32 addr = instr->ctx.addr.addrs[j];
> +
> +				if (j < 2)
> +					addr1 |= addr << (8 * j);
> +				else
> +					addr2 |= addr << (8 * (j - 2));
> +			}
> +
> +			ctrl1 |= CAFE_NAND_CTRL1_HAS_ADDR |
> +				 CAFE_FIELD_PREP(NAND_CTRL1, NUM_ADDR_CYC,
> +						 instr->ctx.addr.naddrs - 1);
> +			cafe_writel(cafe, addr1, NAND_ADDR1);
> +			if (instr->ctx.addr.naddrs > 2)
> +				cafe_writel(cafe, addr2, NAND_ADDR2);

Maybe it is safer to always write this register, no? I don't know if
the IP clears registers between operations. If it does not, you might
end up sending extra command cycles.

> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			data_instr = instr;
> +			ctrl1 |= CAFE_NAND_CTRL1_HAS_DATA_IN;
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			data_instr = instr;
> +			ctrl1 |= CAFE_NAND_CTRL1_HAS_DATA_OUT;
> +			cafe_write_buf(chip, instr->ctx.data.buf.out,
> +				       instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			wait |= CAFE_NAND_IRQ_FLASH_RDY;
> +			waitrdy = true;
> +			break;
> +		}
> +	}
> +
> +	if (data_instr)
> +		cafe_writel(cafe, data_instr->ctx.data.len, NAND_DATA_LEN);
> +
> +	if (cafe->usedma && data_instr) {
> +		u32 dmactrl = CAFE_NAND_DMA_CTRL_ENABLE |
> +			      CAFE_NAND_DMA_CTRL_RESERVED;
> +
> +		wait |= CAFE_NAND_IRQ_DMA_DONE;
> +		dmactrl |= CAFE_FIELD_PREP(NAND_DMA_CTRL, DATA_LEN,
> +					   data_instr->ctx.data.len);
> +		if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN)
> +			dmactrl |= CAFE_NAND_DMA_CTRL_DATA_IN;
> +
> +		cafe_writel(cafe, dmactrl, NAND_DMA_CTRL);
> +	}
> +
> +	/* Clear the pending interrupts before starting the operation. */
> +	cafe_writel(cafe, wait, NAND_IRQ);
> +
> +	cafe_writel(cafe, ctrl2, NAND_CTRL2);
> +	cafe_writel(cafe, ctrl1, NAND_CTRL1);
> +
> +	ret = readl_poll_timeout(cafe->mmio + CAFE_NAND_IRQ, status,
> +				 (status & wait) == wait, 1, USEC_PER_SEC);
> +	if (ret)
> +		return ret;
> +
> +	if (ctrl1 & CAFE_NAND_DMA_CTRL_DATA_IN)
> +		cafe_read_buf(chip, data_instr->ctx.data.buf.in,
> +			      data_instr->ctx.data.len);

As you are limiting the amount of data to 2112B and the number of
address cycles to 5, you should probably use the core's helper
nand_subop_data_len, nand_subob_data_buf and nand_subop_addr_len in
this function.

> +
> +	return 0;
> +}
> +
> +static const struct nand_op_parser cafe_nand_op_parser = NAND_OP_PARSER(
> +	NAND_OP_PARSER_PATTERN(cafe_nand_exec_subop,
> +			       NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +			       NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> +			       NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +			       NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +			       NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2112)),
> +	NAND_OP_PARSER_PATTERN(cafe_nand_exec_subop,
> +			       NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +			       NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> +			       NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +			       NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2112),
> +			       NAND_OP_PARSER_PAT_WAITRDY_ELEM(true))
> +);
> +
> +static int cafe_nand_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op,
> +			     bool check_only)
> +{

I didn't check but are you sure there is no chip-select/timings
handling to do here?

> +	return nand_op_parser_exec_op(chip, &cafe_nand_op_parser, op,
> +				      check_only);
> +}
> +
>  static const struct nand_controller_ops cafe_nand_controller_ops = {
>  	.attach_chip = cafe_nand_attach_chip,
>  	.detach_chip = cafe_nand_detach_chip,
> +	.exec_op = cafe_nand_exec_op,
>  };
>  
>  static void cafe_nand_init(struct cafe_priv *cafe)

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/