| Message ID | 1496836352-8016-7-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Wed, 7 Jun 2017 20:52:15 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > The denali_cmdfunc() actually does nothing valuable for > NAND_CMD_{PAGEPROG,READ0,SEQIN}. > > For NAND_CMD_{READ0,SEQIN}, it copies "page" to "denali->page", then > denali_read_page() and denali_read_page_raw() compare them to check > if the NAND framework called the callbacks in correct order. > (Inconsistently, this check is missing from the denali_write_page() > and denali_write_page_raw().) > > The framework is widely tested by many drivers, so this kind of > sanity check is unneeded. The Denali controller is equipped with > high level interface for read/write, so let's skip unneeded call > of cmdfunc(). I recently changed the semantic of ecc->write_page[_raw]() when NAND_ECC_CUSTOM_PAGE_ACCESS is set [1]. I'm not sure your driver waits for the program command to finish. I think you should wait for INTR_STATUS__PROGRAM_COMP instead of INTR_STATUS__DMA_CMD_COMP in write_page() [2], as is done in write_oob_data(). Note that, even though you listen to INTR_STATUS__PROGRAM_FAIL, you never test the value of irq_status when it != 0, which means you don't detect PROG failures. [1]http://git.infradead.org/l2-mtd.git/commit/41145649f4acb30249b636b945053db50c9331c5 [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/denali.c#L1029 > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: > - Newly added > > drivers/mtd/nand/denali.c | 29 ++++++++--------------------- > 1 file changed, 8 insertions(+), 21 deletions(-) > > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index 991924b9ae2c..1897fe238290 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -998,7 +998,7 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op) > * configuration details. > */ > static int write_page(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf, bool raw_xfer) > + const uint8_t *buf, int page, bool raw_xfer) > { > struct denali_nand_info *denali = mtd_to_denali(mtd); > dma_addr_t addr = denali->buf.dma_buf; > @@ -1006,6 +1006,8 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t irq_status; > uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL; > > + denali->page = page; > + > /* > * if it is a raw xfer, we want to disable ecc and send the spare area. > * !raw_xfer - enable ecc > @@ -1059,7 +1061,7 @@ static int denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, > * for regular page writes, we let HW handle all the ECC > * data written to the device. > */ > - return write_page(mtd, chip, buf, false); > + return write_page(mtd, chip, buf, page, false); > } > > /* > @@ -1075,7 +1077,7 @@ static int denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > * for raw page writes, we want to disable ECC and simply write > * whatever data is in the buffer. > */ > - return write_page(mtd, chip, buf, true); > + return write_page(mtd, chip, buf, page, true); > } > > static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip, > @@ -1105,12 +1107,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, > unsigned long uncor_ecc_flags = 0; > int stat = 0; > > - if (page != denali->page) { > - dev_err(denali->dev, > - "IN %s: page %d is not equal to denali->page %d", > - __func__, page, denali->page); > - BUG(); > - } > + denali->page = page; > > setup_ecc_for_xfer(denali, true, false); > > @@ -1154,12 +1151,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > size_t size = mtd->writesize + mtd->oobsize; > uint32_t irq_mask = INTR__DMA_CMD_COMP; > > - if (page != denali->page) { > - dev_err(denali->dev, > - "IN %s: page %d is not equal to denali->page %d", > - __func__, page, denali->page); > - BUG(); > - } > + denali->page = page; > > setup_ecc_for_xfer(denali, false, true); > denali_enable_dma(denali, true); > @@ -1238,8 +1230,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, > int i; > > switch (cmd) { > - case NAND_CMD_PAGEPROG: > - break; > case NAND_CMD_STATUS: > read_status(denali); > break; > @@ -1259,10 +1249,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, > write_byte_to_buf(denali, id); > } > break; > - case NAND_CMD_READ0: > - case NAND_CMD_SEQIN: > - denali->page = page; > - break; > case NAND_CMD_RESET: > reset_bank(denali); > break; > @@ -1603,6 +1589,7 @@ int denali_init(struct denali_nand_info *denali) > > mtd_set_ooblayout(mtd, &denali_ooblayout_ops); > > + chip->ecc.options |= NAND_ECC_CUSTOM_PAGE_ACCESS; > chip->ecc.read_page = denali_read_page; > chip->ecc.read_page_raw = denali_read_page_raw; > chip->ecc.write_page = denali_write_page;
Hi Boris 2017-06-07 22:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Wed, 7 Jun 2017 20:52:15 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> The denali_cmdfunc() actually does nothing valuable for >> NAND_CMD_{PAGEPROG,READ0,SEQIN}. >> >> For NAND_CMD_{READ0,SEQIN}, it copies "page" to "denali->page", then >> denali_read_page() and denali_read_page_raw() compare them to check >> if the NAND framework called the callbacks in correct order. >> (Inconsistently, this check is missing from the denali_write_page() >> and denali_write_page_raw().) >> >> The framework is widely tested by many drivers, so this kind of >> sanity check is unneeded. The Denali controller is equipped with >> high level interface for read/write, so let's skip unneeded call >> of cmdfunc(). > > I recently changed the semantic of ecc->write_page[_raw]() when > NAND_ECC_CUSTOM_PAGE_ACCESS is set [1]. I'm not sure your driver waits > for the program command to finish. > I think you should wait for INTR_STATUS__PROGRAM_COMP instead of > INTR_STATUS__DMA_CMD_COMP in write_page() [2], as is done in > write_oob_data(). Thanks for the pointer. I missed your commit because I usually develop based on Linus' tree instead of linux-next. I will fix this commit.
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 991924b9ae2c..1897fe238290 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -998,7 +998,7 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op) * configuration details. */ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf, bool raw_xfer) + const uint8_t *buf, int page, bool raw_xfer) { struct denali_nand_info *denali = mtd_to_denali(mtd); dma_addr_t addr = denali->buf.dma_buf; @@ -1006,6 +1006,8 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, uint32_t irq_status; uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL; + denali->page = page; + /* * if it is a raw xfer, we want to disable ecc and send the spare area. * !raw_xfer - enable ecc @@ -1059,7 +1061,7 @@ static int denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, * for regular page writes, we let HW handle all the ECC * data written to the device. */ - return write_page(mtd, chip, buf, false); + return write_page(mtd, chip, buf, page, false); } /* @@ -1075,7 +1077,7 @@ static int denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, * for raw page writes, we want to disable ECC and simply write * whatever data is in the buffer. */ - return write_page(mtd, chip, buf, true); + return write_page(mtd, chip, buf, page, true); } static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip, @@ -1105,12 +1107,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, unsigned long uncor_ecc_flags = 0; int stat = 0; - if (page != denali->page) { - dev_err(denali->dev, - "IN %s: page %d is not equal to denali->page %d", - __func__, page, denali->page); - BUG(); - } + denali->page = page; setup_ecc_for_xfer(denali, true, false); @@ -1154,12 +1151,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, size_t size = mtd->writesize + mtd->oobsize; uint32_t irq_mask = INTR__DMA_CMD_COMP; - if (page != denali->page) { - dev_err(denali->dev, - "IN %s: page %d is not equal to denali->page %d", - __func__, page, denali->page); - BUG(); - } + denali->page = page; setup_ecc_for_xfer(denali, false, true); denali_enable_dma(denali, true); @@ -1238,8 +1230,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, int i; switch (cmd) { - case NAND_CMD_PAGEPROG: - break; case NAND_CMD_STATUS: read_status(denali); break; @@ -1259,10 +1249,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, write_byte_to_buf(denali, id); } break; - case NAND_CMD_READ0: - case NAND_CMD_SEQIN: - denali->page = page; - break; case NAND_CMD_RESET: reset_bank(denali); break; @@ -1603,6 +1589,7 @@ int denali_init(struct denali_nand_info *denali) mtd_set_ooblayout(mtd, &denali_ooblayout_ops); + chip->ecc.options |= NAND_ECC_CUSTOM_PAGE_ACCESS; chip->ecc.read_page = denali_read_page; chip->ecc.read_page_raw = denali_read_page_raw; chip->ecc.write_page = denali_write_page;
The denali_cmdfunc() actually does nothing valuable for NAND_CMD_{PAGEPROG,READ0,SEQIN}. For NAND_CMD_{READ0,SEQIN}, it copies "page" to "denali->page", then denali_read_page() and denali_read_page_raw() compare them to check if the NAND framework called the callbacks in correct order. (Inconsistently, this check is missing from the denali_write_page() and denali_write_page_raw().) The framework is widely tested by many drivers, so this kind of sanity check is unneeded. The Denali controller is equipped with high level interface for read/write, so let's skip unneeded call of cmdfunc(). Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: - Newly added drivers/mtd/nand/denali.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)