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 <[email protected]>
---
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
Hi Sascha,
[email protected] 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
[email protected] wrote on Thu, 16 May 2024 10:13:50 +0200:
> Hi Sascha,
>
> [email protected] 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:
>
> [email protected] wrote on Thu, 16 May 2024 10:13:50 +0200:
>
> > Hi Sascha,
> >
> > [email protected] 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 |
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