| Message ID | 20240514134140.1050141-1-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: rawnand: onfi: read parameter pages in one go | expand |
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
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
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
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
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];
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(-)