[PATCH] mtd: rawnand: onfi: read parameter pages in one go

[PATCH] mtd: rawnand: onfi: read parameter pages in one go

[Sascha Hauer]

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. Instead of relying on these functions
just read the three parameter pages in one go.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/nand_onfi.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 861975e44b552..df1750692404d 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -148,7 +148,6 @@ int nand_onfi_detect(struct nand_chip *chip)
 	struct nand_memory_organization *memorg;
 	struct nand_onfi_params *p = NULL, *pbuf;
 	struct onfi_params *onfi;
-	bool use_datain = false;
 	int onfi_version = 0;
 	char id[4];
 	int i, ret, val;
@@ -166,25 +165,11 @@ int nand_onfi_detect(struct nand_chip *chip)
 	if (!pbuf)
 		return -ENOMEM;
 
-	if (!nand_has_exec_op(chip) || chip->controller->supported_op.data_only_read)
-		use_datain = true;
+	ret = nand_read_param_page_op(chip, 0, pbuf, sizeof(*pbuf) * ONFI_PARAM_PAGES);
+	if (ret)
+		return ret;
 
 	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
-		if (!i)
-			ret = nand_read_param_page_op(chip, 0, &pbuf[i],
-						      sizeof(*pbuf));
-		else if (use_datain)
-			ret = nand_read_data_op(chip, &pbuf[i], sizeof(*pbuf),
-						true, false);
-		else
-			ret = nand_change_read_column_op(chip, sizeof(*pbuf) * i,
-							 &pbuf[i], sizeof(*pbuf),
-							 true);
-		if (ret) {
-			ret = 0;
-			goto free_onfi_param_page;
-		}
-
 		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254);
 		if (crc == le16_to_cpu(pbuf[i].crc)) {
 			p = &pbuf[i];
-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: onfi: read parameter pages in one go

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Tue, 14 May 2024 15:41:40 +0200:

> 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.

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). TBH I have never seen such a device myself, so I wonder
how spread they still are.

What may not be supported however are the DATA_IN cycles.

> Instead of relying on these functions
> just read the three parameter pages in one go.

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.

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.

I believe we should do something like:

nand_read_param_page_op(0)
if (corrupted) {
	if (supported.datain)
		data_in(); /* this is faster */
	else		
		nand_read_param_page_op(1)
}

I'll try to draft something (also applies to the jedec discovery).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: onfi: read parameter pages in one go

[Miquel Raynal]

miquel.raynal@bootlin.com wrote on Thu, 16 May 2024 10:13:50 +0200:

> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Tue, 14 May 2024 15:41:40 +0200:
> 
> > 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.  
> 
> 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.
> 
> What may not be supported however are the DATA_IN cycles.
> 
> > Instead of relying on these functions
> > just read the three parameter pages in one go.  
> 
> 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.
> 
> 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.
> 
> I believe we should do something like:
> 
> nand_read_param_page_op(0)
> if (corrupted) {
> 	if (supported.datain)
> 		data_in(); /* this is faster */
> 	else		
> 		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/qcom_nandc.c#L2965

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: onfi: read parameter pages in one go

[Sascha Hauer]

On Thu, May 16, 2024 at 11:58:12AM +0200, Miquel Raynal wrote:
> 
> miquel.raynal@bootlin.com wrote on Thu, 16 May 2024 10:13:50 +0200:
> 
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Tue, 14 May 2024 15:41:40 +0200:
> > 
> > > 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.  
> > 
> > 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).

I could imagine that there are no ONFI compliant chips with small pages,
I don't know how to verify this though.

Anyway, this was a try to make reading the parameter pages a bit more
straight forward. It seems the MXC NAND controller can't support reading
three parameter pages in one go either. It works in software ECC mode
because I reverse the syndome type layout the controller introduces, but
it can't work with hardware ECC. So disregard this patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: onfi: read parameter pages in one go

[Miquel Raynal]

Hi Sascha,

> > > > 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.    
> > > 
> > > 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).  
> 
> I could imagine that there are no ONFI compliant chips with small pages,
> I don't know how to verify this though.
> 
> Anyway, this was a try to make reading the parameter pages a bit more
> straight forward. It seems the MXC NAND controller can't support reading
> three parameter pages in one go either. It works in software ECC mode
> because I reverse the syndome type layout the controller introduces, but
> it can't work with hardware ECC.

Hardware ECC should not be used at this step and should be disabled.
There should not be any ECC interference.

> So disregard this patch.

I now have a proposal, I'll send a v2 of my previous series.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/