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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Mon, 27 Apr 2020 21:59:33 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

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

Will remove those WARN_ON()s.

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

The number of cycles is controller by NUM_ADDR_CYC so that shouldn't be
a problem, but you're right, writing ADDR2 unconditionally shouldn't
hurt.

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

Indeed. I'll fix that.

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

Apparently no. Having the CS propagated to subops is enough here (CS
selection is done in the subop handler).



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