nand_flash_detect_onfi error
In the file drivers/mtd/nand/nand_base.c, in function nand_flash_detect_onfi reading the ONFI data can be run up to 3 times when there is a CRC error detected in the onfi data:
The function call chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located outside the first for loop:
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
for (i = 0; i < 3; i++) {
for (j = 0; j < sizeof(*p); j++)
((uint8_t *)p)[j] = chip->read_byte(mtd);
if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
le16_to_cpu(p->crc)) {
break;
}
}This results in a read beyond buffer error if the data have to be read more than once.
This error can also be found in U-boot and barebox.
Quoted 17 lines · Renaud Barbier
In the file
drivers/mtd/nand/nand_base.c, in functionnand_flash_detect_onfireading the ONFI data can be run up to 3 times when there is a CRC error detected in the onfi data:The function call
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located outside the first for loop:
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);for (i = 0; i < 3; i++) { for (j = 0; j < sizeof(*p); j++) ((uint8_t *)p)[j] = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break; } }
Quoted 5 lines
This results in a read beyond buffer error if the data have to be read more than once.
This error can also be found in U-boot and barebox.
Hi Renaud,
NAND_CMD_PARAM is supposed to read one parameter page,
plus the two redundant parameter pages as well. That's why the NAND core code reading sizeof(parameter page) x 3 bytes safely.
If you take a look at the ONFI 3.0 spec you'll see the parameter page is specified to be 767 bytes long, i.e. three redundant parameter pages can be read.
If you are seeing this kind of bug with a NAND controller, then you need to extend your CMD_PARAM read to fetch all the copies of the parameter page.
Signature · 2 lines
Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
Ezequiel, Renaud,
Quoted 4 lines · On Tue, 3 Nov 2015 13:56:16 -0300 Ezequiel Garcia…
Quoted 3 lines · Renaud Barbier
In the file
drivers/mtd/nand/nand_base.c, in functionnand_flash_detect_onfireading the ONFI data can be run up to 3 times when there is a CRC error detected in the onfi data:
Do you mean parameter pages (including redundant ones) should be read several times in case of failures or are you talking about redundant parameter pages (there should be at least 2 of them and there can be more)?
I didn't find any mention to the former statement, so if this is really what you meant could you point the section where it's described?
If you're talking about the latter, then the current implementation is correct, because redundant pages are contiguous to the first parameter page.
Quoted 14 lines
Quoted 14 lines
The function call
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located outside the first for loop:
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);for (i = 0; i < 3; i++) { for (j = 0; j < sizeof(*p); j++) ((uint8_t *)p)[j] = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break; } }
Quoted 2 lines
Quoted 2 lines
This results in a read beyond buffer error if the data have to be read more than once.
I don't think so (but I might be wrong). As explained, the redundant parameter pages are concatenated to the main one, so it's perfectly valid to read all of them with a single NAND_CMD_PARAM sequence.
BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
Quoted 10 lines
Quoted 3 lines
This error can also be found in U-boot and barebox.
Hi Renaud,
NAND_CMD_PARAMis supposed to read one parameter page,
plus the two redundant parameter pages as well. That's why the NAND core code reading sizeof(parameter page) x 3 bytes safely.
Quoted 7 lines
If you take a look at the ONFI 3.0 spec you'll see the parameter page is specified to be 767 bytes long, i.e. three redundant parameter pages can be read.
If you are seeing this kind of bug with a NAND controller, then you need to extend your
CMD_PARAMread to fetch all the copies of the parameter page.
Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to retrieve any data before ->read_xxx() is called. I know some controllers retrieve data ahead of time and then provide the previously stored data when ->read_buf() is called, but that's not a good practice to assume it will work this way on all controllers (actually I keep thinking the sane implementations are those waiting for the ->read_buf() call before starting retrieving the data from the NAND chip).
Best Regards,
Boris
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 13 lines · Boris Brezillon
Quoted 11 lines · On Tue, 3 Nov 2015 13:56:16 -0300 Ezequiel Garcia…
Quoted 10 lines · Renaud Barbier
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);for (i = 0; i < 3; i++) { for (j = 0; j < sizeof(*p); j++) ((uint8_t *)p)[j] = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break; } }
...
Quoted 1 line
BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
commit bd9c6e99b58255b9de1982711ac9487c9a2f18be
Author: Brian Norris <[email protected]> Date: Fri Nov 29 22:04:28 2013 -0800
mtd: nand: don't use read_buf for 8-bit ONFI transfers
Brian
Quoted 14 lines · On Tue, 3 Nov 2015 12:45:11 -0800 Brian Norris <[email protected]>…
Quoted 13 lines · Boris Brezillon
Quoted 11 lines · On Tue, 3 Nov 2015 13:56:16 -0300 Ezequiel Garcia…
Quoted 10 lines · Renaud Barbier
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);for (i = 0; i < 3; i++) { for (j = 0; j < sizeof(*p); j++) ((uint8_t *)p)[j] = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break; } }
Quoted 8 lines
...
Quoted 1 line
BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
commit
bd9c6e99b58255b9de1982711ac9487c9a2f18beAuthor: Brian Norris <[email protected]> Date: Fri Nov 29 22:04:28 2013 -0800mtd: nand: don't use
read_buffor 8-bit ONFI transfers
Makes sense. Thanks for pointing this out.
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 20 lines · Boris Brezillon
Quoted 17 lines · On Tue, 3 Nov 2015 12:45:11 -0800 Brian Norris <[email protected]>…
Quoted 13 lines · Boris Brezillon
Quoted 11 lines · On Tue, 3 Nov 2015 13:56:16 -0300 Ezequiel Garcia…
Quoted 10 lines · Renaud Barbier
chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);for (i = 0; i < 3; i++) { for (j = 0; j < sizeof(*p); j++) ((uint8_t *)p)[j] = chip->read_byte(mtd); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { break; } }...
Quoted 1 line
BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
Quoted 7 lines
Quoted 5 lines
commit
bd9c6e99b58255b9de1982711ac9487c9a2f18beAuthor: Brian Norris <[email protected]> Date: Fri Nov 29 22:04:28 2013 -0800mtd: nand: don't use
read_buffor 8-bit ONFI transfersMakes sense. Thanks for pointing this out.
Perhaps it was worth a code comment. It can be confusing.
On 3 November 2015 at 17:25, Boris Brezillon
<[email protected]> wrote:
[..]
Quoted 10 lines
Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to retrieve any data before ->read_xxx() is called. I know some controllers retrieve data ahead of time and then provide the previously stored data when ->read_buf() is called, but that's not a good practice to assume it will work this way on all controllers (actually I keep thinking the sane implementations are those waiting for the ->read_buf() call before starting retrieving the data from the NAND chip).
Right. We've discussed this in the past, and although I don't recall the details,
I recall you were right.
In any case, I just wanted to mention that if Renaud is getting a read beyond the buffer, then probably the driver needs fixing.
Signature · 2 lines
Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
Quoted 14 lines · On Tue, 3 Nov 2015 17:59:01 -0300 Ezequiel Garcia…
On 3 November 2015 at 17:25, Boris Brezillon
<[email protected]> wrote:
[..]Quoted 10 lines
Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to retrieve any data before ->read_xxx() is called. I know some controllers retrieve data ahead of time and then provide the previously stored data when ->read_buf() is called, but that's not a good practice to assume it will work this way on all controllers (actually I keep thinking the sane implementations are those waiting for the ->read_buf() call before starting retrieving the data from the NAND chip).
Quoted 7 lines
Right. We've discussed this in the past, and although I don't recall the details,
I recall you were right.In any case, I just wanted to mention that if Renaud is getting a read beyond the buffer, then probably the driver needs fixing.
My bad, I thought the "read beyond the buffer" thing mentioned by Renaud was something generic. Now I get your point, and I agree: it should be fixed in the NAND controller driver, either by increasing the read buffer or by reworking the implementation to delay data retrieval until ->read_buf() is called.
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 25 lines · Boris Brezillon
Quoted 14 lines · On Tue, 3 Nov 2015 17:59:01 -0300 Ezequiel Garcia…
On 3 November 2015 at 17:25, Boris Brezillon
<[email protected]> wrote:
[..]Quoted 10 lines
Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to retrieve any data before ->read_xxx() is called. I know some controllers retrieve data ahead of time and then provide the previously stored data when ->read_buf() is called, but that's not a good practice to assume it will work this way on all controllers (actually I keep thinking the sane implementations are those waiting for the ->read_buf() call before starting retrieving the data from the NAND chip).
Quoted 7 lines
Right. We've discussed this in the past, and although I don't recall the details,
I recall you were right.In any case, I just wanted to mention that if Renaud is getting a read beyond the buffer, then probably the driver needs fixing.
Quoted 6 lines
My bad, I thought the "read beyond the buffer" thing mentioned by Renaud was something generic. Now I get your point, and I agree: it should be fixed in the NAND controller driver, either by increasing the read buffer or by reworking the implementation to delay data retrieval until ->read_buf() is called.
My CPU is a Freescale P1014 supported by the driver drivers/mtd/nand/fsl_ifc_nand.c. Based on your information it seems the easiest path is to set the "read_bytes" count to 3 * 256 and program the controller to use this byte count when the CMD_PARAM command is used.
Cheers.
Quoted 4 lines · Renaud Barbier
My CPU is a Freescale P1014 supported by the driver
drivers/mtd/nand/fsl_ifc_nand.c. Based on your information it seems the easiest path is to set the "read_bytes" count to 3 * 256 and program the controller to use this byte count when theCMD_PARAMcommand is used.
That's still a bit ugly. It'd be nice not to have to assume the read length...
Also, have you looked at NAND_CMD_RNDOUT support? That's required if you need to read out the extended parameter page. See nand_flash_detect_ext_param_page().
Brian