Received-SPF: pass (google.com: domain of linux-kernel+bounces-180875-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 16 May 2024 11:58:12 +0200
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, Richard
 Weinberger <richard@nod.at>
Subject: Re: [PATCH] mtd: rawnand: onfi: read parameter pages in one go
Message-ID: <20240516115812.02908822@xps-13>
In-Reply-To: <20240516101350.78e5ee29@xps-13>
References: <20240514134140.1050141-1-s.hauer@pengutronix.de>
	<20240516101350.78e5ee29@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 Thu, 16 May 2024 10:13:50 +0200:

> Hi Sascha,
>=20
> s.hauer@pengutronix.de wrote on Tue, 14 May 2024 15:41:40 +0200:
>=20
> > nand_read_data_op() is not supported by all NAND controllers.
> > nand_change_read_column_op() is not supported or at least is hard to
> > support by NAND controllers that use a different page layout than
> > expected by the NAND core. =20
>=20
> I'm sorry but RNDOUT is not so hard to support, and I know no NAND
> controller without this feature (I think even the first mxc controller
> supports it?). However, the command does not exist on small page NANDs
> (512 bytes).

Nevermind, the ONFI spec (in all versions) states that RNDOUT are
allowed during parameter page reads, regardless of the size of the chip
(at least, that is not mentioned).

> TBH I have never seen such a device myself, so I wonder
> how spread they still are.
>=20
> What may not be supported however are the DATA_IN cycles.
>=20
> > Instead of relying on these functions
> > just read the three parameter pages in one go. =20
>=20
> Bitflips in parameter pages are very rare, they are normally quite
> robust. The proposed solution impacts *all* NANDs, because the I/O
> chip speed is at its lowest. There is no reason in most cases to do
> that.
>=20
> I agree there is a problem with the patch I proposed and we need to
> settle. And we simply cannot make RNDOUT calls randomly here as long as
> we want to support small page NANDs.
>=20
> I believe we should do something like:
>=20
> nand_read_param_page_op(0)
> if (corrupted) {
> 	if (supported.datain)
> 		data_in(); /* this is faster */
> 	else	=09
> 		nand_read_param_page_op(1)

This can't work, there is a single address byte. The parameter page
being 256 bytes, we can't use the address parameter.

So I'm sorry but the solutions are:
- DATA_IN cycles (not always supported but the best)
or if unsupported:
- RNDOUT

Re-reading the three pages would be slower and is not supported by all
controllers anyway. Example of controller not supporting it: qcom [1].
That's why I want the constraints to be fairly well described in the
parser.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/qco=
m_nandc.c#L2965

Thanks,
Miqu=C3=A8l