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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Thu, 7 May 2020 17:45:59 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> > > +	}
> > > +
> > > +	return steps;    
> > 
> > I guess you have a limit on steps. It's probably worth checking
> > that steps is in bounds.  
> 
> The upper limit is 2048, I'm not sure it is relevant to add a check
> here?

Well, it wouldn't hurt to add it, just for correctness.

> >   
> > > +}
> > > +
> > > +/* 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.  
> 
> Mmmh probably not, I will test that.
> 
> But a FIFO read is 4 bytes long so anyway, it will probably read/write
> more no matter what I request (and move the SRAM pointer).

The FIFO/SRAM pointer and actual DATA len are most of the time not
correlated, meaning that you can write/read more to/from the FIFO/SRAM
without having extra DATA cycles issued on the bus.

> > > +static const struct nand_op_parser anfc_op_parser = NAND_OP_PARSER(
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_param_read_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC),
> > > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> > > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_param_write_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC),
> > > +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, ANFC_MAX_PARAM_SIZE)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_data_read_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC),
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> > > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_data_write_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC),
> > > +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, ANFC_MAX_CHUNK_SIZE),
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_reset_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_erase_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC),
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_status_type_exec,
> > > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)),
> > > +	NAND_OP_PARSER_PATTERN(
> > > +		anfc_wait_type_exec,
> > > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > +	);
> > > +    
> > 
> > Okay, no DATA-only patterns, so my suggestion to split non-aligned data
> > reads doesn't work. I'd suggest to describe data-lengths
> > constraints rather than automatically adjusting the data length to
> > something bigger when we can't do exactly the number of requested DATA
> > cycles.  
> 
> Well, we *must* adjust the data length automatically. But the below
> change is interesting and should be extended and then this controller
> updated (see the next sentence).

What's probably as important as allowing controllers to exceed the
amount of DATA cycles is flagging operations where that's allowed. I
can think of any READ/WRITE operations where you can issue a
RNDOUT/RNDIN to move the pointer after reading/writing data. READID
would also qualify here as data are just wrapping around, and I think
SET/GET_FEATURES allow that too, but I'm not sure.

Note that the mxc driver is probably even worse in that it only allows
512byte reads/writes, so we'll need the feature if we want to convert
that one.

> 
> > I started doing something similar here [1], except you'd need
> > much more fined-grained constraints, so maybe we should add an optional
> > check hook to data patterns.  
> 
> We could describe a "round_up" limitation too. That's definitely
> something that we can add in this driver on top of [1].
> 
> Would apply to Marvell NFC as well for instance.

Until we have that working, may I suggest to return ENOTSUPP when you
can't issue exactly the number of DATA cycles requested? That implies
doing an extra check to make sure any DATA instruction is either
smaller than MAX_PKT_SIZE or has a valid NUM_PKTS divisor.

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