Received-SPF: pass (google.com: domain of linux-kernel+bounces-177243-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 13 May 2024 09:19:02 +0200
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Richard Weinberger <richard@nod.at>, Vignesh Raghavendra
 <vigneshr@ti.com>, linux-mtd@lists.infradead.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] mtd: nand: mxc_nand: implement exec_op
Message-ID: <20240513091902.2c00d30a@xps-13>
In-Reply-To: <20240508-mtd-nand-mxc-nand-exec-op-v2-2-6b7366b7831f@pengutronix.de>
References: <20240508-mtd-nand-mxc-nand-exec-op-v2-0-6b7366b7831f@pengutronix.de>
	<20240508-mtd-nand-mxc-nand-exec-op-v2-2-6b7366b7831f@pengutronix.de>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Sascha,

> @@ -1717,9 +1465,111 @@ static int mxcnd_setup_interface(struct nand_chip=
 *chip, int chipnr,
>  	return host->devtype_data->setup_interface(chip, chipnr, conf);
>  }
> =20
> +static int mxcnd_exec_op(struct nand_chip *chip,
> +			 const struct nand_operation *op,
> +			 bool check_only)
> +{
> +	struct mxc_nand_host *host =3D nand_get_controller_data(chip);
> +	struct mtd_info *mtd =3D nand_to_mtd(chip);
> +	int i, j, buf_len;
> +	void *buf_read =3D NULL;
> +	const void *buf_write =3D NULL;
> +	const struct nand_op_instr *instr;
> +	bool readid =3D false;
> +	bool statusreq =3D false;
> +
> +	dev_dbg(host->dev, "%s: %d instructions\n", __func__, op->ninstrs);

Maybe you want to get rid of this debug line.

> +
> +	if (check_only)
> +		return 0;
> +
> +	for (i =3D 0; i < op->ninstrs; i++) {
> +		instr =3D &op->instrs[i];
> +
> +		nand_op_trace("  ", instr);
> +
> +		switch (instr->type) {
> +		case NAND_OP_WAITRDY_INSTR:
> +			/*
> +			 * NFC handles R/B internally. Therefore, this function
> +			 * always returns status as ready.

This is no longer a standalone function, maybe:

"The controller handles the R/B pin internally, therefore there is
nothing to do here."

> +			 */
> +			break;
> +		case NAND_OP_CMD_INSTR:
> +			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_PAGEPROG)
> +				host->devtype_data->send_page(mtd, NFC_INPUT);
> +
> +			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> +
> +			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_READID)
> +				readid =3D true;
> +			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_STATUS)
> +				statusreq =3D true;
> +
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +			for (j =3D 0; j < instr->ctx.addr.naddrs; j++) {
> +				bool islast =3D j =3D=3D instr->ctx.addr.naddrs - 1;
> +				host->devtype_data->send_addr(host, instr->ctx.addr.addrs[j], islast=
);
> +			}
> +			break;
> +		case NAND_OP_DATA_OUT_INSTR:
> +			buf_write =3D instr->ctx.data.buf.out;
> +			buf_len =3D instr->ctx.data.len;
> +
> +			memcpy32_toio(host->main_area0, buf_write, buf_len);
> +			if (chip->oob_poi)
> +				copy_spare(mtd, false, chip->oob_poi);

This copy should not be needed. It should be in your page accessors if
needed.

> +
> +			break;
> +		case NAND_OP_DATA_IN_INSTR:
> +
> +			buf_read =3D instr->ctx.data.buf.in;
> +			buf_len =3D instr->ctx.data.len;
> +
> +			if (readid) {
> +				host->devtype_data->send_read_id(host);
> +				readid =3D false;
> +
> +				memcpy32_fromio(host->data_buf, host->main_area0, buf_len * 2);
> +
> +				if (chip->options & NAND_BUSWIDTH_16) {
> +					u8 *bufr =3D buf_read;
> +					u16 *bufw =3D host->data_buf;
> +					for (j =3D 0; j < buf_len; j++)
> +						bufr[j] =3D bufw[j];
> +				} else {
> +					memcpy(buf_read, host->data_buf, buf_len);
> +				}
> +				break;
> +			}
> +
> +			if (statusreq) {
> +				*(u8*)buf_read =3D host->devtype_data->get_dev_status(host);
> +				statusreq =3D false;
> +				break;
> +			}
> +
> +			host->devtype_data->read_page(chip);
> +
> +			if (IS_ALIGNED(buf_len, 4)) {
> +				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +			} else {
> +				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> +				memcpy(buf_read, host->data_buf, buf_len);
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

Otherwise I'm very happy with the look.

Thanks,
Miqu=C3=A8l