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

Hi Sascha,

> > > > +		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." =20
>=20
> Ok.
>=20
> >=20
> > And this is actually very wrong.
> >=20
> > You should call wait_op_done() instead. =20
>=20
> No, I don't think so. wait_op_done() is called to wait for the interrupt
> of the controller indicating a basic operation is done. A basic operation
> can be that a command is being sent or an address byte has been sent to
> the chip during an address cycle.
>=20
> With this arbitrary example:
>=20
> 	struct nand_op_instr instrs[] =3D {
> 		NAND_OP_CMD(NAND_CMD_READ0, 0),
> 		NAND_OP_ADDR(4, addrs, 0),
> 		NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
> 		NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
> 				 NAND_COMMON_TIMING_NS(conf, tRR_min)),
> 		NAND_OP_DATA_IN(len, buf, 0),
> 	};
>=20
> I'll call wait_op_done() once for NAND_CMD_READ0, four times for the NAND=
_OP_ADDR,
> then once again for the NAND_CMD_READSTART command and four times (on i.M=
X27, once
> per subpage) for the NAND_OP_DATA_IN operation. Calling wait_op_done() fo=
r the
> NAND_OP_WAIT_RDY operation woul only timeout because there is no operatio=
n in flight
> currently.

Ah, ok, makes sense as well. If you want (this is not mandatory) you
may want to rename the function wait_instr_done() to fit the core's
naming: an operation being an aggregate of instructions/cycles.

Thanks,
Miqu=C3=A8l