Hi Boris,
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Thu, 7 May
2020 18:11:39 +0200:
> 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.
Sure!
