Received-SPF: pass (google.com: domain of linux-kernel+bounces-177263-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 13 May 2024 09:32:56 +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: <20240513093256.363861d1@xps-13>
In-Reply-To: <20240513091902.2c00d30a@xps-13>
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>
	<20240513091902.2c00d30a@xps-13>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable


miquel.raynal@bootlin.com wrote on Mon, 13 May 2024 09:19:02 +0200:

> Hi Sascha,
>=20
> > @@ -1717,9 +1465,111 @@ static int mxcnd_setup_interface(struct nand_ch=
ip *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); =
=20
>=20
> Maybe you want to get rid of this debug line.
>=20
> > +
> > +	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. =20
>=20
> This is no longer a standalone function, maybe:
>=20
> "The controller handles the R/B pin internally, therefore there is
> nothing to do here."

And this is actually very wrong.

You should call wait_op_done() instead.

>=20
> > +			 */
> > +			break;
> > +		case NAND_OP_CMD_INSTR:
> > +			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_PAGEPROG)
> > +				host->devtype_data->send_page(mtd, NFC_INPUT);

Actually this is not the right place. You should trigger the transfer
from controller SRAM to NAND (and the other way around) in the
NAND_OP_DATA_OUT_INSTR case.

Here you should just call ->send_cmd.

> > +
> > +			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], isla=
st);
> > +			}
> > +			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); =20
>=20
> This copy should not be needed. It should be in your page accessors if
> needed.
>=20
> > +
> > +			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;
> > +} =20
>=20
> Otherwise I'm very happy with the look.
>=20
> Thanks,
> Miqu=C3=A8l


Thanks,
Miqu=C3=A8l