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

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

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Thu, 7 May
2020 14:51:27 +0200:

> > +/* NAND framework ->exec_op() hooks and related helpers */
> > +static void anfc_parse_instructions(struct nand_chip *chip,
> > +				    const struct nand_subop *subop,
> > +				    struct anfc_op *nfc_op)
> > +{
> > +	struct anand *anand = to_anand(chip);
> > +	const struct nand_op_instr *instr = NULL;
> > +	bool first_cmd = true;
> > +	unsigned int op_id;
> > +	int i;
> > +
> > +	memset(nfc_op, 0, sizeof(*nfc_op));
> > +	nfc_op->addr2_reg = ADDR2_CS(anand->cs);
> > +	nfc_op->cmd_reg = CMD_PAGE_SIZE(anand->page_sz);
> > +
> > +	for (op_id = 0; op_id < subop->ninstrs; op_id++) {
> > +		unsigned int offset, naddrs, pktsize;
> > +		const u8 *addrs;
> > +		u8 *buf;
> > +
> > +		instr = &subop->instrs[op_id];
> > +
> > +		switch (instr->type) {
> > +		case NAND_OP_CMD_INSTR:
> > +			if (first_cmd)
> > +				nfc_op->cmd_reg |= CMD_1(instr->ctx.cmd.opcode);
> > +			else
> > +				nfc_op->cmd_reg |= CMD_2(instr->ctx.cmd.opcode);
> > +
> > +			first_cmd = false;
> > +			break;
> > +
> > +		case NAND_OP_ADDR_INSTR:
> > +			offset = nand_subop_get_addr_start_off(subop, op_id);
> > +			naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
> > +			addrs = &instr->ctx.addr.addrs[offset];
> > +			nfc_op->cmd_reg |= CMD_NADDRS(naddrs);
> > +
> > +			for (i = 0; i < min(ANFC_MAX_ADDR_CYC, naddrs); i++) {
> > +				if (i < 4)
> > +					nfc_op->addr1_reg |= (u32)addrs[i] << i * 8;
> > +				else
> > +					nfc_op->addr2_reg |= addrs[i];
> > +			}
> > +
> > +			break;
> > +		case NAND_OP_DATA_IN_INSTR:
> > +			nfc_op->read = true;
> > +			fallthrough;
> > +		case NAND_OP_DATA_OUT_INSTR:
> > +			offset = nand_subop_get_data_start_off(subop, op_id);
> > +			buf = instr->ctx.data.buf.in;
> > +			nfc_op->buf = &buf[offset];
> > +			nfc_op->len = nand_subop_get_data_len(subop, op_id);
> > +			nfc_op->steps = anfc_len_to_steps(chip, nfc_op->len);
> > +			pktsize = DIV_ROUND_UP(nfc_op->len, nfc_op->steps);
> > +			nfc_op->pkt_reg |= PKT_SIZE(round_up(pktsize, 4)) |  
> 
> Hm, pktsize has to be aligned on 4? Again, that's not great since you
> adjust the size without letting the core know you did that.

I confirm the round_up() is needed. It does not work without.

> 
> > +					   PKT_STEPS(nfc_op->steps);
> > +			break;
> > +		case NAND_OP_WAITRDY_INSTR:
> > +			nfc_op->rdy_timeout_ms = instr->ctx.waitrdy.timeout_ms;
> > +			break;
> > +		}
> > +	}
> > +}