Received-SPF: pass (google.com: domain of linux-kernel+bounces-177592-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 13 May 2024 14:55:36 +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: <20240513145536.71145b65@xps-13>
In-Reply-To: <ZkHYo6-8SC4RMlsG@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>
	<ZkHYo6-8SC4RMlsG@pengutronix.de>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Sascha,

> > > > +	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." =20
> >=20
> > And this is actually very wrong.
> >=20
> > You should call wait_op_done() instead.
> >  =20
> > >  =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); =20
> >=20
> > 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.
> >=20
> > Here you should just call ->send_cmd. =20
>=20
> I tried to get away here with using the standard nand_write_page_raw()
> function. This does multiple NAND_OP_DATA_OUT_INSTR ops before finally
> sending a NAND_CMD_PAGEPROG command. With software BCH ECC I collect all
> data being written in memory and copy it to the controller SRAM en bloc.
> I have to do this after the final NAND_OP_DATA_OUT_INSTR op. During a
> NAND_OP_DATA_OUT_INSTR I don't know if this is going to be the last one
> or if other NAND_OP_DATA_OUT_INSTR ops follow, so I do the copy to SRAM
> right before a NAND_CMD_PAGEPROG.

I fully understand the mindset, but we had too many issues with this
this using ->cmdfunc() and ->cmd_ctrl(), so we introduced ->exec_op()
exactly for this purpose: controller drivers shall not try to guess
what the core wants. The core now provides the full operation and also a
way to check if the operation is supported.

So if something is unsupported, just report it is unsupported.

> I could move the ->send_page() call to NAND_OP_DATA_OUT_INSTR, but then
> I have to overwrite the ecc->write_page_raw() hook.

I guess your issue is the core trying to write the in-band data
and later the oob data. I totally get why you're doing that now, makes
sense, but this is not how ->exec_op() should be implemented, let me
explain (and propose a solution).

In general, it is preferred to have a clean ->exec_op()
implementation (in case we later extend the core) that is capable of
doing the ops correctly and *just* what has been requested. Drivers
should refuse the operations they don't support instead of trying
to work-arouding them. So here the correct way is probably to provide a
->write_page_raw() hook to simply avoid the unsupported ops. In your
case you just need to reference nand_monolithic_write_page_raw() and
you're done.

Also please check the cases you don't support and refuse them in
->exec_op() (both in the normal *and* in the check_only path). This is
also part of ->exec_op()'s design and is very important. I believe you
should take inspiration from the pl35x-nand-controller.c driver which
typically covers all the known-to-be-working cases with just three
patterns filled in a parser structure (true/false means the instruction
is mandatory or can be skipped, please have a look to
pl35x_nandc_op_parser). You will notice all cases in the parser
reference the same function in the end (you can do the same). This way
you know only supported patterns will be used and the core can easily
check whether something "new" is supported or not. Right now the
current mxc ->exec_op() implementation says it supports all
instructions, which is of course a lie.

Side note: Core helpers were initially written with the more flexible
approach, ie. splitting the operations in smaller pieces. This
typically does not work with some controllers such as yours. What I
want is the driver telling the core it should not try to split the ops
if that's the case. I know the core is a bit flacky regarding these
checks. There is currently only the data_only_read supported_op check
that is used for early discovery (see nand_onfi_detect()), but in case
you have issues with this type of situation, let me know, we (I) can
improve the core.

> Also it must be
> clear that the NAND core will never do multiple NAND_OP_DATA_OUT_INSTR
> when programming a page.

That's typically something that could happen (in practice I don't have
any plans for that, this is theoretically speaking), hence using the
nand_op_parser structure will ensure if we ever extend the core, the
driver won't break.

> Side note: I decided to collect the page data in a memory buffer rather
> than in the controller SRAM directly because it seemed too complicated
> and error prone to find the correct offset in SRAM for random column
> addresses. Also the SRAM can only do word and halfword accesses, so I
> additionally would have to emulate byte accesses with read-modify-write
> halfword accesses. While certainly doable I'd like to defer this to a
> future optimisation exercise.

Oh yeah, totally agreed. I saw the implementation and I agree we should
aim for a working driver. Then if you want to improve it for
performance reasons, changes will be welcome.

Thanks,
Miqu=C3=A8l