Thank you Miquel, comments below:
> Sent: Friday, September 22, 2023 at 7:24 AM
> From: "Miquel Raynal" <miquel.raynal at bootlin.com>
> To: dregan at mail.com
> Cc: bcm-kernel-feedback-list at broadcom.com, linux-mtd at lists.infradead.org, f.fainelli at gmail.com, rafal at milecki.pl, joel.peshkin at broadcom.com, computersforpeace at gmail.com, dan.beygelman at broadcom.com, william.zhang at broadcom.com, frieder.schrempf at kontron.de, linux-kernel at vger.kernel.org, vigneshr at ti.com, richard at nod.at, bbrezillon at kernel.org, kdasu.kdev at gmail.com
> Subject: Re: [PATCH] mtd: rawnand: brcmnand: Initial exec_op implementation
>
> Hello,
>
> dregan at mail.com wrote on Wed, 13 Sep 2023 04:24:41 +0200:
>
> > Initial exec_op implementation for Broadcom STB, Broadband and iProc SoC
> > This adds exec_op and removes the legacy interface.
>
> Thanks a lot for the conversion, a few comments below.
>
...
> > +static void brcmnand_exec_instr(struct brcmnand_host *host,
> > + const struct nand_op_instr *instr,
> > + bool last_op)
> > +{
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > + unsigned int i;
> > + const u8 *out;
> > + u8 *in;
> > +
> > + bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> > +
> > + switch (instr->type) {
> > + case NAND_OP_CMD_INSTR:
> > + brcmnand_low_level_op(host, LL_OP_CMD,
> > + instr->ctx.cmd.opcode, last_op);
> > + break;
> > +
> > + case NAND_OP_ADDR_INSTR:
> > + for (i = 0; i < instr->ctx.addr.naddrs; i++)
> > + brcmnand_low_level_op(host, LL_OP_ADDR,
> > + instr->ctx.addr.addrs[i],
> > + last_op);
> > + break;
> > +
> > + case NAND_OP_DATA_IN_INSTR:
> > + in = instr->ctx.data.buf.in;
> > + for (i = 0; i < instr->ctx.data.len; i++) {
> > + brcmnand_low_level_op(host, LL_OP_RD, 0, last_op);
> > + in[i] = brcmnand_read_reg(host->ctrl,
> > + BRCMNAND_LL_RDATA);
> > + }
> > + break;
> > +
> > + case NAND_OP_DATA_OUT_INSTR:
> > + out = instr->ctx.data.buf.out;
> > + for (i = 0; i < instr->ctx.data.len; i++)
> > + brcmnand_low_level_op(host, LL_OP_WR, out[i], last_op);
> > + break;
> > +
> > + default:
>
> Should probably return an error.
>
Will do.
> > + break;
> > + }
> > +}
> > +
> > +static int brcmnand_parser_exec_matched_op(struct nand_chip *chip,
> > + const struct nand_subop *subop)
> > +{
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + const struct nand_op_instr *instr = &subop->instrs[0];
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + static int erase; /* will initialize to zero */
> > + static int status; /* will initialize to zero */
>
> Looks really suspicious, I believe you should rework your
> implementation to avoid static variables here.
>
Will move these flags into our local structure.
> > +
> > + for (i = 0; i < subop->ninstrs; i++) {
> > + instr = &subop->instrs[i];
> > +
> > + if ((instr->type == NAND_OP_CMD_INSTR) &&
> > + (instr->ctx.cmd.opcode == NAND_CMD_STATUS))
> > + status = 1;
> > + else if (status && (instr->type == NAND_OP_DATA_IN_INSTR)) {
> > + /*
> > + * need to fake the nand device write protect because nand_base does a
> > + * nand_check_wp which calls nand_status_op NAND_CMD_STATUS which checks
> > + * that the nand is not write protected before an operation starts.
> > + * The problem with this is it's done outside exec_op so the nand is
> > + * write protected and this check will fail until the write or erase
> > + * or write back operation actually happens where we turn off wp.
> > + */
>
> If there is a problem with the core it needs to be handled in the core,
> not workarounded here. The whole logic with the status property seems
> really wrong.
>
I'm trying to change our current code functionality as little as
possible by having this function in the same way as it always has
and I do not want to make changes too much outside the scope
of this exec_op change.
> > + u8 *in;
> > +
> > + status = 0;
> > +
> > + instr = &subop->instrs[i];
> > + in = instr->ctx.data.buf.in;
> > + in[0] = brcmnand_status(host) | NAND_STATUS_WP; /* hide WP status */
> > + } else { /* otherwise pass to low level implementation */
> > + if ((instr->type == NAND_OP_CMD_INSTR) &&
> > + (instr->ctx.cmd.opcode == NAND_CMD_RESET)) {
> > + erase = 0;
> > + status = 0;
> > + }
> > +
> > + if (erase) {
> > + erase = 0;
> > + brcmnand_wp(mtd, 1);
> > + }
> > +
> > + if ((instr->type == NAND_OP_CMD_INSTR) &&
> > + (instr->ctx.cmd.opcode == NAND_CMD_ERASE1))
> > + brcmnand_wp(mtd, 0);
> > +
> > + if ((instr->type == NAND_OP_CMD_INSTR) &&
> > + (instr->ctx.cmd.opcode == NAND_CMD_ERASE2))
> > + erase = 1;
> > +
> > + brcmnand_exec_instr(host, instr, i == (subop->ninstrs - 1));
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const struct nand_op_parser brcmnand_op_parser = NAND_OP_PARSER(
> > + /* false means the element MUST match,
> > + * true means it does not have to match, catch all patterns
> > + */
>
> I believe the comment is not really useful here?
>
Will remove.
...
>
> Thanks,
> Miquèl
>
Thanks!
-Dave