[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231120112816.15a19e05@xps-13>
Date: Mon, 20 Nov 2023 11:28:16 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: dregan@...adcom.com
Cc: bcm-kernel-feedback-list@...adcom.com,
linux-mtd@...ts.infradead.org, f.fainelli@...il.com,
rafal@...ecki.pl, joel.peshkin@...adcom.com,
computersforpeace@...il.com, dan.beygelman@...adcom.com,
william.zhang@...adcom.com, frieder.schrempf@...tron.de,
linux-kernel@...r.kernel.org, vigneshr@...com, richard@....at,
bbrezillon@...nel.org, kdasu.kdev@...il.com,
JaimeLiao <jaimeliao.tw@...il.com>,
Adam Borowski <kilobyte@...band.pl>
Subject: Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation
Hello,
dregan@...adcom.com wrote on Mon, 23 Oct 2023 10:14:44 -0700:
> From: David Regan <dregan@...adcom.com>
>
> exec_op implementation for Broadcom STB, Broadband and iProc SoC
> This adds exec_op and removes the legacy interface. Based on changes
> proposed by Boris Brezillon.
>
> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
>
> Signed-off-by: David Regan <dregan@...adcom.com>
I'm fine with patches 1-3, a few minor nits on this version as well,
nothing big. I guess I'll let some time to Florian as well to give his
feedback and perhaps test the ->exec_op() implementation.
...
> +static int brcmnand_exec_instr(struct brcmnand_host *host, int i,
> + const struct nand_operation *op)
> +{
> + struct brcmnand_controller *ctrl = host->ctrl;
> + const struct nand_op_instr *instr = &op->instrs[i];
> + const u8 *out;
> + u8 *in;
> + int ret = 0;
> + bool last_op;
> +
> + /*
> + * if we are on the last command in the sequence (not including
> + * waitrdy which is not a NAND command) then flag the controller
May I suggest:
/*
* The controller needs to be aware of the last command in the operation
* (WAITRDY excepted).
*/
> + */
> + last_op = (((i == (op->ninstrs - 1)) &&
> + (instr->type != NAND_OP_WAITRDY_INSTR)) ||
You can cross the 80 chars boundary. Please use this form:
last_op = ((i == (op->ninstrs - 1)) && (instr->type != NAND_OP_WAITRDY_INSTR)) ||
((i == (op->ninstrs - 2)) && (op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR));
> + ((i == (op->ninstrs - 2)) &&
> + (op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR)));
> +
> + 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 &&
> + i == (instr->ctx.addr.naddrs - 1));
> + 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 &&
> + i == (instr->ctx.data.len - 1));
> + 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 &&
> + i == (instr->ctx.data.len - 1));
> + break;
> +
> + case NAND_OP_WAITRDY_INSTR:
> + ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> + break;
> +
> + default:
> + dev_err(ctrl->dev, "unsupported instruction type: %d\n",
> + instr->type);
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int brcmnand_exec_op_is_status(const struct nand_operation *op)
brcmnand_op_is_status() would make more sense
> +{
> + if ((op->ninstrs == 2) &&
> + (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> + (op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
> + (op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
> + return 1;
> +
> + return 0;
> +}
> +
> +static int brcmnand_exec_op_is_reset(const struct nand_operation *op)
same here, please s/exec_//
> +{
> + if ((op->ninstrs == 1) &&
> + (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> + (op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
> + return 1;
> +
> + return 0;
> +}
> +
> +static int brcmnand_exec_op(struct nand_chip *chip,
> + const struct nand_operation *op,
> + bool check_only)
> +{
> + struct brcmnand_host *host = nand_get_controller_data(chip);
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + u8 *status;
> + unsigned int i;
> + int ret = 0;
> +
> + if (check_only)
> + return 0;
> +
> + if (brcmnand_exec_op_is_status(op)) {
> + status = op->instrs[1].ctx.data.buf.in;
> + *status = brcmnand_status(host);
> +
> + return 0;
> + }
I would add the below chunk here:
} else if (brcmnand_exec_op_is_reset(op)) {
...
return ...
}
> +
> + if (op->deassert_wp)
> + brcmnand_wp(mtd, 0);
> +
> + for (i = 0; i < op->ninstrs; i++) {
> + ret = brcmnand_exec_instr(host, i, op);
> + if (ret)
> + break;
> + }
> +
> + if (op->deassert_wp)
> + brcmnand_wp(mtd, 1);
> +
> + if (brcmnand_exec_op_is_reset(op)) {
> + brcmnand_wp(mtd, 1);
> + brcmnand_status(host);
> + }
> +
> + return ret;
> +}
Thanks,
Miquèl
Powered by blists - more mailing lists