[U-Boot] [PATCH 1/2] mtd: denali: improve nand_read_oob and fix nand_write_oob
Chin Liang See
clsee at altera.com
Wed Apr 23 08:50:03 CEST 2014
Hi Masahiro,
I am back :)
Sorry for late reply as I was aways for few weeks.
On Fri, 2014-04-18 at 20:30 +0900, Masahiro Yamada wrote:
> This patch is a review feedback against Denali NAND controller driver.
> http://patchwork.ozlabs.org/patch/333077/
>
> This is not applicable to the mainline.
>
> ---
>
> Hi Chin,
>
> This patch fixes some issues.
>
> [1] Fix denali_write_oob() handler.
>
> As for v7, "nand markbad" did not work at all.
>
> With this patch, it works.
Actually this was tested before.
Here is the test result (as this was written as SOCFPGA documentation)
SOCFPGA_CYCLONE5 # nand markbad 0
Bad block table written to 0x00003ffe0000, version 0x02
Bad block table written to 0x00003ffc0000, version 0x02
block 0x00000000 successfully marked as bad
SOCFPGA_CYCLONE5 # nand bad
Device 0 bad blocks:
00000000
3ff80000
3ffa0000
3ffc0000
3ffe0000
SOCFPGA_CYCLONE5 #
>
> [2] Make denali_read_oob() 10x faster.
>
> One of the fatal issues of v7 is "nand bad" command is extremely slow.
>
> This is the benchmark of v7
>
> => time nand bad
>
> Device 0 bad blocks:
>
> time: 11.300 seconds, 11300 ticks
>
> It is really really painful to wait more than 10 seconds just for bad block
> scanning to boot Linux.
>
> In v7, denali_read_oob() calls denali_read_page_raw().
> This causes the transfering main area data and memcpy of it,
> which leads to significant performance regression.
>
> Like Linux Kernel, dedicated denali_read_oob() must be impilemented.
>
> With this patch, "nand bad" command gets much faster!
>
> This is my benchmark:
>
> => time nand bad
>
> Device 0 bad blocks:
>
> time: 0.998 seconds, 998 ticks
I believe the bad block scanning would be required for new chip only.
This is when the bad block table is not existing within the NAND chip.
Once its available, the scanning would not be required as the BBT would
be loaded into memory. From there, the read and write would run much
much faster. Nevertheless, these code will help to speed up the bad
block scanning for the first time. I can apply this to v8 once I get
your feedback on other comments.
>
> [3] Remove false comment
>
> /* Writes OOB data to the device.
> * This code unused under normal U-Boot console as normally page write raw
> * to be used for write oob data with main data.
> */
> static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
>
> This comment is telling a lie.
> write_oob_data() is called from "nand markbad" command.
> It must be deleted.
Actually this comment is correct during integration into 2013 release. I
set the option to use NAND_USE_FLASH_BBT where it will not call
nand_do_write_oob. Do note that we will always write bad block info into
bad block table which are located at last 4 blocks of the NAND flash.
In latest code, the function "nand_do_write_oob" will be called if
write_oob is set (where NAND_BBT_NO_OOB_BBM is clear). From the
description of NAND_BBT_NO_OOB_BBM, user need to set it if we don't want
the bad block table located at OOB.
For our case, it should be true as we are using HW ECC. Our data might
span into OOB region. Definitely we don't want this bad block table
overwritten our data then. Wonder is this match with your understanding.
In short, the function write_oob_data shouldn't be called for our case.
Thanks
Chin Liang
>
> Signed-off-by: Masahiro Yamada <yamada.m at jp.panasonic.com>
> Cc: Chin Liang See <clsee at altera.com>
> Cc: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
> Cc: David Woodhouse <David.Woodhouse at intel.com>
> Cc: Brian Norris <computersforpeace at gmail.com>
> Cc: Scott Wood <scottwood at freescale.com>
> ---
> drivers/mtd/nand/denali.c | 136 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 91 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 348e244..dcde3e6 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -527,46 +527,34 @@ static void setup_ecc_for_xfer(bool ecc_en, bool transfer_spare)
> static int denali_send_pipeline_cmd(bool ecc_en, bool transfer_spare,
> int access_type, int op)
> {
> - uint32_t addr = 0x0, cmd = 0x0, irq_status = 0, irq_mask = 0;
> - uint32_t page_count = 1; /* always read a page */
> -
> - if (op == DENALI_READ)
> - irq_mask = INTR_STATUS__LOAD_COMP;
> - else if (op == DENALI_WRITE)
> - irq_mask = INTR_STATUS__PROGRAM_COMP |
> - INTR_STATUS__PROGRAM_FAIL;
> - else
> - BUG();
> + uint32_t addr = 0x0, cmd = 0x0, irq_status;
> + static uint32_t page_count = 1;
> +
> + setup_ecc_for_xfer(ecc_en, transfer_spare);
>
> /* clear interrupts */
> clear_interrupts();
>
> - /* setup ECC and transfer spare reg */
> - setup_ecc_for_xfer(ecc_en, transfer_spare);
> -
> addr = BANK(denali.flash_bank) | denali.page;
>
> /* setup the acccess type */
> cmd = MODE_10 | addr;
> - index_addr((uint32_t)cmd, access_type);
> + index_addr(cmd, access_type);
>
> /* setup the pipeline command */
> - if (access_type == SPARE_ACCESS && op == DENALI_WRITE)
> - index_addr((uint32_t)cmd, DENALI_BUFFER_WRITE);
> - else if (access_type == SPARE_ACCESS && op == DENALI_READ)
> - index_addr((uint32_t)cmd, DENALI_BUFFER_LOAD);
> - else
> - index_addr((uint32_t)cmd, 0x2000 | op | page_count);
> + index_addr(cmd, 0x2000 | op | page_count);
>
> - /* wait for command to be accepted */
> - irq_status = wait_for_irq(irq_mask);
> - if ((irq_status & irq_mask) != irq_mask)
> - return -EIO;
> + cmd = MODE_01 | addr;
> + writel(cmd, denali.flash_mem + INDEX_CTRL_REG);
>
> - if (access_type != SPARE_ACCESS) {
> - cmd = MODE_01 | addr;
> - writel(cmd, denali.flash_mem + INDEX_CTRL_REG);
> + if (op == DENALI_READ) {
> + /* wait for command to be accepted */
> + irq_status = wait_for_irq(INTR_STATUS__LOAD_COMP);
> +
> + if (irq_status == 0)
> + return -EIO;
> }
> +
> return 0;
> }
>
> @@ -586,6 +574,29 @@ static int write_data_to_flash_mem(const void *buf, int len)
> return i * 4; /* intent is to return the number of bytes read */
> }
>
> +/* helper function that simply reads a buffer from the flash */
> +static int read_data_from_flash_mem(uint8_t *buf, int len)
> +{
> + uint32_t i, *buf32;
> +
> + /*
> + * we assume that len will be a multiple of 4, if not
> + * it would be nice to know about it ASAP rather than
> + * have random failures...
> + * This assumption is based on the fact that this
> + * function is designed to be used to read flash pages,
> + * which are typically multiples of 4...
> + */
> +
> + BUG_ON((len % 4) != 0);
> +
> + /* transfer the data from the flash */
> + buf32 = (uint32_t *)buf;
> + for (i = 0; i < len / 4; i++)
> + *buf32++ = readl(denali.flash_mem + INDEX_DATA_REG);
> + return i * 4; /* intent is to return the number of bytes read */
> +}
> +
> static void denali_mode_main_access(void)
> {
> uint32_t addr, cmd;
> @@ -602,29 +613,64 @@ static void denali_mode_main_spare_access(void)
> index_addr(cmd, MAIN_SPARE_ACCESS);
> }
>
> -/* Writes OOB data to the device.
> - * This code unused under normal U-Boot console as normally page write raw
> - * to be used for write oob data with main data.
> - */
> +/* writes OOB data to the device */
> static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
> {
> - uint32_t cmd;
> + uint32_t irq_status;
> + uint32_t irq_mask = INTR_STATUS__PROGRAM_COMP |
> + INTR_STATUS__PROGRAM_FAIL;
> + int status = 0;
>
> denali.page = page;
> - debug("* write_oob_data *\n");
>
> - /* We need to write to buffer first through MAP00 command */
> - cmd = MODE_00 | BANK(denali.flash_bank);
> - writel(cmd, denali.flash_mem + INDEX_CTRL_REG);
> + if (denali_send_pipeline_cmd(false, true, SPARE_ACCESS,
> + DENALI_WRITE) == 0) {
> + write_data_to_flash_mem(buf, mtd->oobsize);
>
> - /* send the data into flash buffer */
> - write_data_to_flash_mem(buf, mtd->oobsize);
> + /* wait for operation to complete */
> + irq_status = wait_for_irq(irq_mask);
>
> - /* activate the write through MAP10 commands */
> - if (denali_send_pipeline_cmd(false, false, SPARE_ACCESS, DENALI_WRITE))
> - return -EIO;
> + if (irq_status == 0) {
> + dev_err(denali->dev, "OOB write failed\n");
> + status = -EIO;
> + }
> + } else {
> + printf("unable to send pipeline command\n");
> + status = -EIO;
> + }
> + return status;
> +}
>
> - return 0;
> +/* reads OOB data from the device */
> +static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
> +{
> + uint32_t irq_mask = INTR_STATUS__LOAD_COMP,
> + irq_status = 0, addr = 0x0, cmd = 0x0;
> +
> + denali.page = page;
> +
> + if (denali_send_pipeline_cmd(false, true, SPARE_ACCESS,
> + DENALI_READ) == 0) {
> + read_data_from_flash_mem(buf, mtd->oobsize);
> +
> + /* wait for command to be accepted
> + * can always use status0 bit as the mask is identical for each
> + * bank. */
> + irq_status = wait_for_irq(irq_mask);
> +
> + if (irq_status == 0)
> + printf("page on OOB timeout %d\n", denali.page);
> +
> + /* We set the device back to MAIN_ACCESS here as I observed
> + * instability with the controller if you do a block erase
> + * and the last transaction was a SPARE_ACCESS. Block erase
> + * is reliable (according to the MTD test infrastructure)
> + * if you are in MAIN_ACCESS.
> + */
> + addr = BANK(denali.flash_bank) | denali.page;
> + cmd = MODE_10 | addr;
> + index_addr(cmd, MAIN_ACCESS);
> + }
> }
>
> /* this function examines buffers to see if they contain data that
> @@ -908,9 +954,9 @@ static uint8_t denali_read_byte(struct mtd_info *mtd)
> static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> int page)
> {
> - debug("denali_read_oob at page %08x\n", page);
> - denali.page = page;
> - return denali_read_page_raw(mtd, chip, denali.buf.buf, 1, page);
> + read_oob_data(mtd, chip->oob_poi, page);
> +
> + return 0;
> }
>
> static void denali_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
More information about the U-Boot
mailing list