Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[Miquel Raynal]

Hello,

dregan@xxxxxxxxxxxx wrote on Mon, 23 Oct 2023 10:14:44 -0700:

> From: David Regan <dregan@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>

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