Re: [U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller
Hi Scott,
Le Mon, 16 Mar 2015 16:30:29 -0500, Scott Wood
scottw...@freescale.com a écrit :
On Sat, 2015-03-14 at 15:27 +0100, Albert ARIBAUD wrote:
Bonjour Scott,
Le Fri, 13 Mar 2015 16:57:33 -0500, Scott Wood
scottw...@freescale.com a écrit :
On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote:
+ /* go through all four small pages */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0,
lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status =
readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY));
How much time does 1 reads of this register equate to? Are you sure
it's enough? Timeouts should generally be in terms of time, not loop
iterations.
I followed the examples in several drivers where timeouts are by
iteration. Note that -- while this does not void your point -- I did
not use 1 but 10, which at a CPU clock of 208 MHz, and assuming
an optimistic one instruction per cycle and two instructions per loop,
makes the loop last at least 960 us, well over the 600 us which the
NAND takes for any page programming.
What if this driver ends up being used on hardware that runs
significantly faster than 208 MHz? I could understand if it's hugely
space-constrained SPL code (like the ones that have to fit in 4K), but
otherwise why not make use of the timekeeping code that exists in U-Boot
(either by reading the timer, or by putting udelay(1) in the loop body)?
The udelay(1) in the loop is ok with me -- I'll put it in v6.
+#define LARGE_PAGE_SIZE 2048
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+ struct lpc32xx_oob oob;
+ unsigned int page = offs / LARGE_PAGE_SIZE;
+ unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE);
+
+ while (left) {
+ int res = read_single_page(dst, page, oob);
+ page++;
+ /* if read succeeded, even if fixed by ECC */
+ if (res = 0) {
+ /* skip bad block */
+ if (oob.free[0].free_oob_bytes[0] != 0xff)
+ continue;
+ if (oob.free[0].free_oob_bytes[1] != 0xff)
+ continue;
+ /* page is good, keep it */
+ dst += LARGE_PAGE_SIZE;
+ left--;
+ }
You should be checking the designated page(s) of the block, rather than
the current page, for the bad block markers -- and skipping the entire
block if it's bad.
Will fix this -- is there any helper function in the bad block
management code for this? I could not find one, but I'm no NAND expert.
I don't know of any such helper -- outside of SPL it's handled via the
BBT. fsl_ifc_spl.c is an example that checks for bad block markers, but
it's hardcoded to assume the first two pages of a block which is a bit
simpler than checking at the end of the block.
Thanks -- I'll dig into the BBT code to see how it's handled in my
target and put a fix in v6 based on that.
-Scott
Cordialement,
Albert ARIBAUD
3ADEV
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller
On Sat, 2015-03-14 at 15:27 +0100, Albert ARIBAUD wrote:
Bonjour Scott,
Le Fri, 13 Mar 2015 16:57:33 -0500, Scott Wood
scottw...@freescale.com a écrit :
On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote:
+ /* go through all four small pages */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0, lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status = readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY));
How much time does 1 reads of this register equate to? Are you sure
it's enough? Timeouts should generally be in terms of time, not loop
iterations.
I followed the examples in several drivers where timeouts are by
iteration. Note that -- while this does not void your point -- I did
not use 1 but 10, which at a CPU clock of 208 MHz, and assuming
an optimistic one instruction per cycle and two instructions per loop,
makes the loop last at least 960 us, well over the 600 us which the
NAND takes for any page programming.
What if this driver ends up being used on hardware that runs
significantly faster than 208 MHz? I could understand if it's hugely
space-constrained SPL code (like the ones that have to fit in 4K), but
otherwise why not make use of the timekeeping code that exists in U-Boot
(either by reading the timer, or by putting udelay(1) in the loop body)?
+#define LARGE_PAGE_SIZE 2048
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+ struct lpc32xx_oob oob;
+ unsigned int page = offs / LARGE_PAGE_SIZE;
+ unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE);
+
+ while (left) {
+ int res = read_single_page(dst, page, oob);
+ page++;
+ /* if read succeeded, even if fixed by ECC */
+ if (res = 0) {
+ /* skip bad block */
+ if (oob.free[0].free_oob_bytes[0] != 0xff)
+ continue;
+ if (oob.free[0].free_oob_bytes[1] != 0xff)
+ continue;
+ /* page is good, keep it */
+ dst += LARGE_PAGE_SIZE;
+ left--;
+ }
You should be checking the designated page(s) of the block, rather than
the current page, for the bad block markers -- and skipping the entire
block if it's bad.
Will fix this -- is there any helper function in the bad block
management code for this? I could not find one, but I'm no NAND expert.
I don't know of any such helper -- outside of SPL it's handled via the
BBT. fsl_ifc_spl.c is an example that checks for bad block markers, but
it's hardcoded to assume the first two pages of a block which is a bit
simpler than checking at the end of the block.
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller
Bonjour Scott,
Le Fri, 13 Mar 2015 16:57:33 -0500, Scott Wood
scottw...@freescale.com a écrit :
On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote:
+ /* go through all four small pages */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0, lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status = readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY));
How much time does 1 reads of this register equate to? Are you sure
it's enough? Timeouts should generally be in terms of time, not loop
iterations.
I followed the examples in several drivers where timeouts are by
iteration. Note that -- while this does not void your point -- I did
not use 1 but 10, which at a CPU clock of 208 MHz, and assuming
an optimistic one instruction per cycle and two instructions per loop,
makes the loop last at least 960 us, well over the 600 us which the
NAND takes for any page programming.
+static int read_single_page(uint8_t *dest, int page,
+ struct lpc32xx_oob *oob)
+{
+ int status, i, timeout, err, max_bitflips = 0;
+
+ /* enter read mode */
+ writel(NAND_CMD_READ0, lpc32xx_nand_mlc_registers-cmd);
+ /* send column (lsb then MSB) and page (lsb to MSB) */
+ writel(0, lpc32xx_nand_mlc_registers-addr);
+ writel(0, lpc32xx_nand_mlc_registers-addr);
+ writel(page 0xff, lpc32xx_nand_mlc_registers-addr);
+ writel((page8) 0xff, lpc32xx_nand_mlc_registers-addr);
+ writel((page16) 0xff, lpc32xx_nand_mlc_registers-addr);
+ /* start reading */
+ writel(NAND_CMD_READSTART, lpc32xx_nand_mlc_registers-cmd);
+
+ /* large page auto decode read */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0, lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status = readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY))
+ ;
+ /* return -1 if hard error */
+ if (status ISR_DECODER_FAILURE)
+ return -1;
+ /* keep count of maximum bitflips performed */
+ if (status ISR_DECODER_ERROR) {
+ err = ISR_DECODER_ERRORS(status);
+ if (err max_bitflips)
+ max_bitflips = err;
+ }
+ /* copy first 512 bytes into buffer */
+ memcpy(dest+i*512, lpc32xx_nand_mlc_registers-buff, 512);
+ /* copy next 6 bytes bytes into OOB buffer */
+ memcpy(oob-free[i], lpc32xx_nand_mlc_registers-buff, 6);
+ }
+ return max_bitflips;
+}
+
Why keep track of max_bitflips if the caller doesn't use it?
Because I modeled read_single_page from another read function, and I
preferred to minimize modifications and keep returning as much info
as I have, which can help debugging and cannot cause harm as it does
not affect the caller indeed.
+#define LARGE_PAGE_SIZE 2048
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+ struct lpc32xx_oob oob;
+ unsigned int page = offs / LARGE_PAGE_SIZE;
+ unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE);
+
+ while (left) {
+ int res = read_single_page(dst, page, oob);
+ page++;
+ /* if read succeeded, even if fixed by ECC */
+ if (res = 0) {
+ /* skip bad block */
+ if (oob.free[0].free_oob_bytes[0] != 0xff)
+ continue;
+ if (oob.free[0].free_oob_bytes[1] != 0xff)
+ continue;
+ /* page is good, keep it */
+ dst += LARGE_PAGE_SIZE;
+ left--;
+ }
You should be checking the designated page(s) of the block, rather than
the current page, for the bad block markers -- and skipping the entire
block if it's bad.
Will fix this -- is there any helper function in the bad block
management code for this? I could not find one, but I'm no NAND expert.
Also, if you fail ECC, that should be a fatal error, not something to
silently skip.
Will fix.
-Scott
Cordialement,
Albert ARIBAUD
3ADEV
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller
On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote:
+ /* go through all four small pages */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0, lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status = readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY));
How much time does 1 reads of this register equate to? Are you sure
it's enough? Timeouts should generally be in terms of time, not loop
iterations.
+static int read_single_page(uint8_t *dest, int page,
+ struct lpc32xx_oob *oob)
+{
+ int status, i, timeout, err, max_bitflips = 0;
+
+ /* enter read mode */
+ writel(NAND_CMD_READ0, lpc32xx_nand_mlc_registers-cmd);
+ /* send column (lsb then MSB) and page (lsb to MSB) */
+ writel(0, lpc32xx_nand_mlc_registers-addr);
+ writel(0, lpc32xx_nand_mlc_registers-addr);
+ writel(page 0xff, lpc32xx_nand_mlc_registers-addr);
+ writel((page8) 0xff, lpc32xx_nand_mlc_registers-addr);
+ writel((page16) 0xff, lpc32xx_nand_mlc_registers-addr);
+ /* start reading */
+ writel(NAND_CMD_READSTART, lpc32xx_nand_mlc_registers-cmd);
+
+ /* large page auto decode read */
+ for (i = 0; i 4; i++) {
+ /* start auto decode (reads 528 NAND bytes) */
+ writel(0, lpc32xx_nand_mlc_registers-ecc_auto_dec_reg);
+ /* wait for controller to return to ready state */
+ timeout = LPC32X_NAND_TIMEOUT;
+ do {
+ if (timeout-- == 0)
+ return -1;
+ status = readl(lpc32xx_nand_mlc_registers-isr);
+ } while (!(status ISR_CONTROLLER_READY))
+ ;
+ /* return -1 if hard error */
+ if (status ISR_DECODER_FAILURE)
+ return -1;
+ /* keep count of maximum bitflips performed */
+ if (status ISR_DECODER_ERROR) {
+ err = ISR_DECODER_ERRORS(status);
+ if (err max_bitflips)
+ max_bitflips = err;
+ }
+ /* copy first 512 bytes into buffer */
+ memcpy(dest+i*512, lpc32xx_nand_mlc_registers-buff, 512);
+ /* copy next 6 bytes bytes into OOB buffer */
+ memcpy(oob-free[i], lpc32xx_nand_mlc_registers-buff, 6);
+ }
+ return max_bitflips;
+}
+
Why keep track of max_bitflips if the caller doesn't use it?
+#define LARGE_PAGE_SIZE 2048
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+ struct lpc32xx_oob oob;
+ unsigned int page = offs / LARGE_PAGE_SIZE;
+ unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE);
+
+ while (left) {
+ int res = read_single_page(dst, page, oob);
+ page++;
+ /* if read succeeded, even if fixed by ECC */
+ if (res = 0) {
+ /* skip bad block */
+ if (oob.free[0].free_oob_bytes[0] != 0xff)
+ continue;
+ if (oob.free[0].free_oob_bytes[1] != 0xff)
+ continue;
+ /* page is good, keep it */
+ dst += LARGE_PAGE_SIZE;
+ left--;
+ }
You should be checking the designated page(s) of the block, rather than
the current page, for the bad block markers -- and skipping the entire
block if it's bad.
Also, if you fail ECC, that should be a fatal error, not something to
silently skip.
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
