| Message ID | 1527250904-21988-14-git-send-email-absahu@codeaurora.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | Update for QCOM NAND driver | expand |
Hi Abhishek, On Fri, 25 May 2018 17:51:41 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > The QCOM NAND controller layout is such that, the bad block byte > offset for last codeword will come to first byte in spare area. "is the first spare byte"? > Currently, the raw read for last codeword is being done with > copy_last_cw function. It does following 2 things: "It does the following:" > > 1. Read the last codeword bytes from NAND chip to NAND > controller internal HW buffer. > 2. Copy all these bytes from HW buffer to actual buffer. > > For bad block check, maximum two bytes are required so instead of > copying the complete bytes in step 2, only those bbm_size bytes > can be copied. > > This patch does minor code reorganization for the same. After > this, copy_last_cw function won’t be required. "This patch does minor code reorganization to just retrieve these two bytes when checking for bad blocks, allowing to remove copy_last_cw() now useless." > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > --- > * Changes from v2: > 1. Changed commit message and comments slightly > > * Changes from v1: > NEW CHANGE > > drivers/mtd/nand/raw/qcom_nandc.c | 66 +++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 41 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index d693b5f..f72bc8a 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -1769,41 +1769,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf, > return parse_read_errors(host, data_buf_start, oob_buf_start); > } > > -/* > - * a helper that copies the last step/codeword of a page (containing free oob) > - * into our local buffer > - */ > -static int copy_last_cw(struct qcom_nand_host *host, int page) > -{ > - struct nand_chip *chip = &host->chip; > - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct nand_ecc_ctrl *ecc = &chip->ecc; > - int size; > - int ret; > - > - clear_read_regs(nandc); > - > - size = host->use_ecc ? host->cw_data : host->cw_size; > - > - /* prepare a clean read buffer */ > - memset(nandc->data_buffer, 0xff, size); > - > - set_address(host, host->cw_size * (ecc->steps - 1), page); > - update_rw_regs(host, 1, true); > - > - config_nand_single_cw_page_read(nandc); > - > - read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0); > - > - ret = submit_descs(nandc); > - if (ret) > - dev_err(nandc->dev, "failed to copy last codeword\n"); > - > - free_descs(nandc); > - > - return ret; > -} > - > /* implements ecc->read_page() */ > static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *buf, int oob_required, int page) > @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) > struct nand_ecc_ctrl *ecc = &chip->ecc; > int page, ret, bbpos, bad = 0; > u32 flash_status; > + u8 *bbm_bytes_buf = chip->data_buf; > > page = (int)(ofs >> chip->page_shift) & chip->pagemask; > > @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) > * that contains the BBM > */ > host->use_ecc = false; > + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); Are we sure there is no layout with only 1 step? > > clear_bam_transaction(nandc); > - ret = copy_last_cw(host, page); > - if (ret) > + clear_read_regs(nandc); > + > + set_address(host, host->cw_size * (ecc->steps - 1), page); > + update_rw_regs(host, 1, true); > + > + /* > + * The last codeword data will be copied from NAND device to NAND > + * controller internal HW buffer. Copy only required BBM size bytes > + * from this HW buffer to bbm_bytes_buf which is present at > + * bbpos offset. > + */ > + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > + config_nand_single_cw_page_read(nandc); > + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, > + host->bbm_size, 0); > + > + ret = submit_descs(nandc); > + free_descs(nandc); > + if (ret) { > + dev_err(nandc->dev, "failed to copy bad block bytes\n"); > goto err; > + } > > flash_status = le32_to_cpu(nandc->reg_read_buf[0]); > > @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) > goto err; > } > > - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > - > - bad = nandc->data_buffer[bbpos] != 0xff; > + bad = bbm_bytes_buf[0] != 0xff; This is suspect as it still points to the beginning of the data buffer. Can you please check you did not meant bbm_bytes_buf[bbpos]? > > if (chip->options & NAND_BUSWIDTH_16) > - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > + bad = bad || (bbm_bytes_buf[1] != 0xff); > err: > return bad; > } Thanks, Miquèl
Hi Abhishek, > @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) > goto err; > } > > - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > - > - bad = nandc->data_buffer[bbpos] != 0xff; > + bad = bbm_bytes_buf[0] != 0xff; BTW, as there are host->bbm_size bytes that can inform on the block state, don't we need to check all of them? > > if (chip->options & NAND_BUSWIDTH_16) > - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > + bad = bad || (bbm_bytes_buf[1] != 0xff); > err: > return bad; > } Thanks, Miquèl
On 2018-05-26 14:16, Miquel Raynal wrote: > Hi Abhishek, > > On Fri, 25 May 2018 17:51:41 +0530, Abhishek Sahu > <absahu@codeaurora.org> wrote: > >> The QCOM NAND controller layout is such that, the bad block byte >> offset for last codeword will come to first byte in spare area. > > "is the first spare byte"? > > >> Currently, the raw read for last codeword is being done with >> copy_last_cw function. It does following 2 things: > > "It does the following:" > >> >> 1. Read the last codeword bytes from NAND chip to NAND >> controller internal HW buffer. >> 2. Copy all these bytes from HW buffer to actual buffer. >> >> For bad block check, maximum two bytes are required so instead of >> copying the complete bytes in step 2, only those bbm_size bytes >> can be copied. >> >> This patch does minor code reorganization for the same. After >> this, copy_last_cw function won’t be required. > > "This patch does minor code reorganization to just retrieve these two > bytes when checking for bad blocks, allowing to remove > copy_last_cw() now useless." > Thanks Miquel. I will update all these. >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >> --- >> * Changes from v2: >> 1. Changed commit message and comments slightly >> >> * Changes from v1: >> NEW CHANGE >> >> drivers/mtd/nand/raw/qcom_nandc.c | 66 >> +++++++++++++++------------------------ >> 1 file changed, 25 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c >> b/drivers/mtd/nand/raw/qcom_nandc.c >> index d693b5f..f72bc8a 100644 >> --- a/drivers/mtd/nand/raw/qcom_nandc.c >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c >> @@ -1769,41 +1769,6 @@ static int read_page_ecc(struct qcom_nand_host >> *host, u8 *data_buf, >> return parse_read_errors(host, data_buf_start, oob_buf_start); >> } >> >> -/* >> - * a helper that copies the last step/codeword of a page (containing >> free oob) >> - * into our local buffer >> - */ >> -static int copy_last_cw(struct qcom_nand_host *host, int page) >> -{ >> - struct nand_chip *chip = &host->chip; >> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); >> - struct nand_ecc_ctrl *ecc = &chip->ecc; >> - int size; >> - int ret; >> - >> - clear_read_regs(nandc); >> - >> - size = host->use_ecc ? host->cw_data : host->cw_size; >> - >> - /* prepare a clean read buffer */ >> - memset(nandc->data_buffer, 0xff, size); >> - >> - set_address(host, host->cw_size * (ecc->steps - 1), page); >> - update_rw_regs(host, 1, true); >> - >> - config_nand_single_cw_page_read(nandc); >> - >> - read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0); >> - >> - ret = submit_descs(nandc); >> - if (ret) >> - dev_err(nandc->dev, "failed to copy last codeword\n"); >> - >> - free_descs(nandc); >> - >> - return ret; >> -} >> - >> /* implements ecc->read_page() */ >> static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip, >> uint8_t *buf, int oob_required, int page) >> @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info >> *mtd, loff_t ofs) >> struct nand_ecc_ctrl *ecc = &chip->ecc; >> int page, ret, bbpos, bad = 0; >> u32 flash_status; >> + u8 *bbm_bytes_buf = chip->data_buf; >> >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; >> >> @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> * that contains the BBM >> */ >> host->use_ecc = false; >> + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > > Are we sure there is no layout with only 1 step? All the layouts are such that, the BBM will come in first byte of spare area. For 4 bit ECC, the cw_size is 528 so for 2K page 2048 - 528 * 3 = 464 So for last CW, the 464 is BBM (i.e 2048th byte) in full page. > >> >> clear_bam_transaction(nandc); >> - ret = copy_last_cw(host, page); >> - if (ret) >> + clear_read_regs(nandc); >> + >> + set_address(host, host->cw_size * (ecc->steps - 1), page); >> + update_rw_regs(host, 1, true); >> + >> + /* >> + * The last codeword data will be copied from NAND device to NAND >> + * controller internal HW buffer. Copy only required BBM size bytes >> + * from this HW buffer to bbm_bytes_buf which is present at >> + * bbpos offset. >> + */ >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> + config_nand_single_cw_page_read(nandc); >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, >> + host->bbm_size, 0); >> + >> + ret = submit_descs(nandc); >> + free_descs(nandc); >> + if (ret) { >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); >> goto err; >> + } >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> goto err; >> } >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> - >> - bad = nandc->data_buffer[bbpos] != 0xff; >> + bad = bbm_bytes_buf[0] != 0xff; > > This is suspect as it still points to the beginning of the data buffer. > Can you please check you did not meant bbm_bytes_buf[bbpos]? > The main thing here is nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); After reading one complete CW from NAND, the data will be still in NAND HW buffer. The above register tells that we need to read data from bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh and 2 byte for 16 bus width) in bbm_bytes_buf. So bbm_bytes_buf[0] will contain the BBM first byte. and bbm_bytes_buf[1] will contain the BBM second byte. Regards, Abhishek >> >> if (chip->options & NAND_BUSWIDTH_16) >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> + bad = bad || (bbm_bytes_buf[1] != 0xff); >> err: >> return bad; >> } > > > Thanks, > Miquèl
On 2018-05-26 14:28, Miquel Raynal wrote: > Hi Abhishek, > > >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> goto err; >> } >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> - >> - bad = nandc->data_buffer[bbpos] != 0xff; >> + bad = bbm_bytes_buf[0] != 0xff; > > BTW, as there are host->bbm_size bytes that can inform on the block > state, don't we need to check all of them? > We are checking all of them. host->bbm_size will be either 1 (for NAND_BUSWIDTH_8) or 2 (for NAND_BUSWIDTH_16). https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/mtd/nand/raw/qcom_nandc.c#L2347 Thanks, Abhishek >> >> if (chip->options & NAND_BUSWIDTH_16) >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> + bad = bad || (bbm_bytes_buf[1] != 0xff); >> err: >> return bad; >> } > > Thanks, > Miquèl
Hi Abhishek, > >> /* implements ecc->read_page() */ > >> static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip, > >> uint8_t *buf, int oob_required, int page) > >> @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info >> *mtd, loff_t ofs) > >> struct nand_ecc_ctrl *ecc = &chip->ecc; > >> int page, ret, bbpos, bad = 0; > >> u32 flash_status; > >> + u8 *bbm_bytes_buf = chip->data_buf; > >> >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; > >> >> @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> * that contains the BBM > >> */ > >> host->use_ecc = false; > >> + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > > > Are we sure there is no layout with only 1 step? > > All the layouts are such that, the BBM will come in > first byte of spare area. > > For 4 bit ECC, the cw_size is 528 so for 2K page > > 2048 - 528 * 3 = 464 My question was more about small page NANDs. But I suppose it works too if ecc->steps == 1. > > So for last CW, the 464 is BBM (i.e 2048th byte) in > full page. > > > >> >> clear_bam_transaction(nandc); > >> - ret = copy_last_cw(host, page); > >> - if (ret) > >> + clear_read_regs(nandc); > >> + > >> + set_address(host, host->cw_size * (ecc->steps - 1), page); > >> + update_rw_regs(host, 1, true); > >> + > >> + /* > >> + * The last codeword data will be copied from NAND device to NAND > >> + * controller internal HW buffer. Copy only required BBM size bytes > >> + * from this HW buffer to bbm_bytes_buf which is present at > >> + * bbpos offset. > >> + */ > >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > >> + config_nand_single_cw_page_read(nandc); > >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, > >> + host->bbm_size, 0); > >> + > >> + ret = submit_descs(nandc); > >> + free_descs(nandc); > >> + if (ret) { > >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); > >> goto err; > >> + } > >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); > >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> goto err; > >> } > >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > >> - > >> - bad = nandc->data_buffer[bbpos] != 0xff; > >> + bad = bbm_bytes_buf[0] != 0xff; > > > This is suspect as it still points to the beginning of the data buffer. > > Can you please check you did not meant bbm_bytes_buf[bbpos]? > > > The main thing here is > nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > > After reading one complete CW from NAND, the data will be still > in NAND HW buffer. > > The above register tells that we need to read data from > bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh > and 2 byte for 16 bus width) in bbm_bytes_buf. I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then it's ok. > > So bbm_bytes_buf[0] will contain the BBM first byte. > and bbm_bytes_buf[1] will contain the BBM second byte. > > Regards, > Abhishek > > >> >> if (chip->options & NAND_BUSWIDTH_16) > >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > >> + bad = bad || (bbm_bytes_buf[1] != 0xff); Sorry, my mistake, I did not see the above line. However, technically, the BBM could be located in the first, second or last page of the block. You should check the three of them are 0xFF before declaring the block is not bad. The more I look at the function, the more I wonder if you actually need it. Why does the generic nand_block_bad() implementation in the core do not fit? > >> err: > >> return bad; > >> } > > > > Thanks, > > Miquèl
Hi Abhishek, On Mon, 28 May 2018 11:46:47 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > On 2018-05-26 14:28, Miquel Raynal wrote: > > Hi Abhishek, > > > >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> goto err; > >> } > >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > >> - > >> - bad = nandc->data_buffer[bbpos] != 0xff; > >> + bad = bbm_bytes_buf[0] != 0xff; > > > BTW, as there are host->bbm_size bytes that can inform on the block > > state, don't we need to check all of them? > > > We are checking all of them. > host->bbm_size will be either 1 (for NAND_BUSWIDTH_8) or > 2 (for NAND_BUSWIDTH_16). > > https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/mtd/nand/raw/qcom_nandc.c#L2347 > > Thanks, > Abhishek > > >> >> if (chip->options & NAND_BUSWIDTH_16) > >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > >> + bad = bad || (bbm_bytes_buf[1] != 0xff); As told in my previous reply, I missed the above line. However, after checking the code of the core (nand_base.c) I wonder if it is useful to check for the second byte. And if you look at the core's implementation you'll see that the offset is not always 0 in the OOB but maybe 5 for small page NAND chips. Please have a look to the generic implementation and tell me why this is really needed? Thanks, Miquèl
On 2018-05-28 12:33, Miquel Raynal wrote: > Hi Abhishek, > >> >> /* implements ecc->read_page() */ >> >> static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip, >> >> uint8_t *buf, int oob_required, int page) >> >> @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info >> *mtd, loff_t ofs) >> >> struct nand_ecc_ctrl *ecc = &chip->ecc; >> >> int page, ret, bbpos, bad = 0; >> >> u32 flash_status; >> >> + u8 *bbm_bytes_buf = chip->data_buf; >> >> >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; >> >> >> @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> * that contains the BBM >> >> */ >> >> host->use_ecc = false; >> >> + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> > > Are we sure there is no layout with only 1 step? >> >> All the layouts are such that, the BBM will come in >> first byte of spare area. >> >> For 4 bit ECC, the cw_size is 528 so for 2K page >> >> 2048 - 528 * 3 = 464 > > My question was more about small page NANDs. But I suppose it works > too if ecc->steps == 1. > Correct Miquel. >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in >> full page. >> >> > >> >> clear_bam_transaction(nandc); >> >> - ret = copy_last_cw(host, page); >> >> - if (ret) >> >> + clear_read_regs(nandc); >> >> + >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); >> >> + update_rw_regs(host, 1, true); >> >> + >> >> + /* >> >> + * The last codeword data will be copied from NAND device to NAND >> >> + * controller internal HW buffer. Copy only required BBM size bytes >> >> + * from this HW buffer to bbm_bytes_buf which is present at >> >> + * bbpos offset. >> >> + */ >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> + config_nand_single_cw_page_read(nandc); >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, >> >> + host->bbm_size, 0); >> >> + >> >> + ret = submit_descs(nandc); >> >> + free_descs(nandc); >> >> + if (ret) { >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); >> >> goto err; >> >> + } >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> goto err; >> >> } >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> >> - >> >> - bad = nandc->data_buffer[bbpos] != 0xff; >> >> + bad = bbm_bytes_buf[0] != 0xff; >> > > This is suspect as it still points to the beginning of the data buffer. >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? >> > >> The main thing here is >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> After reading one complete CW from NAND, the data will be still >> in NAND HW buffer. >> >> The above register tells that we need to read data from >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh >> and 2 byte for 16 bus width) in bbm_bytes_buf. > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then > it's ok. > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. >> and bbm_bytes_buf[1] will contain the BBM second byte. >> >> Regards, >> Abhishek >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); > > Sorry, my mistake, I did not see the above line. > > However, technically, the BBM could be located in the first, second or > last page of the block. You should check the three of them are 0xFF > before declaring the block is not bad. > > The more I look at the function, the more I wonder if you actually need > it. Why does the generic nand_block_bad() implementation in the core > do not fit? The BBM bytes can be accessed in raw mode only for QCOM NAND Contoller. We started with following patch for initial patches http://patchwork.ozlabs.org/patch/508565/ I am also not very much sure, how can we go ahead now. Ideally we need to use generic function only which requires raw_read. Thanks, Abhishek
Hi Abhishek, On Mon, 28 May 2018 15:40:52 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > On 2018-05-28 12:33, Miquel Raynal wrote: > > Hi Abhishek, > > >> >> /* implements ecc->read_page() */ > >> >> static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip, > >> >> uint8_t *buf, int oob_required, int page) > >> >> @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info >> *mtd, loff_t ofs) > >> >> struct nand_ecc_ctrl *ecc = &chip->ecc; > >> >> int page, ret, bbpos, bad = 0; > >> >> u32 flash_status; > >> >> + u8 *bbm_bytes_buf = chip->data_buf; > >> >> >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; > >> >> >> @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> >> * that contains the BBM > >> >> */ > >> >> host->use_ecc = false; > >> >> + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > >> > > Are we sure there is no layout with only 1 step? > >> >> All the layouts are such that, the BBM will come in > >> first byte of spare area. > >> >> For 4 bit ECC, the cw_size is 528 so for 2K page > >> >> 2048 - 528 * 3 = 464 > > > My question was more about small page NANDs. But I suppose it works > > too if ecc->steps == 1. > > > Correct Miquel. > > >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in > >> full page. > >> >> > >> >> clear_bam_transaction(nandc); > >> >> - ret = copy_last_cw(host, page); > >> >> - if (ret) > >> >> + clear_read_regs(nandc); > >> >> + > >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); > >> >> + update_rw_regs(host, 1, true); > >> >> + > >> >> + /* > >> >> + * The last codeword data will be copied from NAND device to NAND > >> >> + * controller internal HW buffer. Copy only required BBM size bytes > >> >> + * from this HW buffer to bbm_bytes_buf which is present at > >> >> + * bbpos offset. > >> >> + */ > >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > >> >> + config_nand_single_cw_page_read(nandc); > >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, > >> >> + host->bbm_size, 0); > >> >> + > >> >> + ret = submit_descs(nandc); > >> >> + free_descs(nandc); > >> >> + if (ret) { > >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); > >> >> goto err; > >> >> + } > >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); > >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> >> goto err; > >> >> } > >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > >> >> - > >> >> - bad = nandc->data_buffer[bbpos] != 0xff; > >> >> + bad = bbm_bytes_buf[0] != 0xff; > >> > > This is suspect as it still points to the beginning of the data buffer. > >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? > >> > > >> The main thing here is > >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > >> >> After reading one complete CW from NAND, the data will be still > >> in NAND HW buffer. > >> >> The above register tells that we need to read data from > >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh > >> and 2 byte for 16 bus width) in bbm_bytes_buf. > > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then > > it's ok. > > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. > >> and bbm_bytes_buf[1] will contain the BBM second byte. > >> >> Regards, > >> Abhishek > >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) > >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); > > > Sorry, my mistake, I did not see the above line. > > > However, technically, the BBM could be located in the first, second or > > last page of the block. You should check the three of them are 0xFF > > before declaring the block is not bad. > > > The more I look at the function, the more I wonder if you actually need > > it. Why does the generic nand_block_bad() implementation in the core > > do not fit? > > The BBM bytes can be accessed in raw mode only for QCOM NAND > Contoller. We started with following patch for initial patches > > http://patchwork.ozlabs.org/patch/508565/ > > I am also not very much sure, how can we go ahead now. > Ideally we need to use generic function only which > requires raw_read. > I see, thanks for pointing this thread. Well for now then let's keep our driver-specific implementation. I will just ask you to do a consistent check as requested above (you can copy code from the core) and add a comment above this function explaining why it is needed (what you just told me). Thanks, Miquèl
On 2018-06-07 18:23, Miquel Raynal wrote: > Hi Abhishek, > > On Mon, 28 May 2018 15:40:52 +0530, Abhishek Sahu > <absahu@codeaurora.org> wrote: > >> On 2018-05-28 12:33, Miquel Raynal wrote: >> > Hi Abhishek, >> > >> >> /* implements ecc->read_page() */ >> >> >> static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip, >> >> >> uint8_t *buf, int oob_required, int page) >> >> >> @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info >> *mtd, loff_t ofs) >> >> >> struct nand_ecc_ctrl *ecc = &chip->ecc; >> >> >> int page, ret, bbpos, bad = 0; >> >> >> u32 flash_status; >> >> >> + u8 *bbm_bytes_buf = chip->data_buf; >> >> >> >> page = (int)(ofs >> chip->page_shift) & chip->pagemask; >> >> >> >> @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> >> * that contains the BBM >> >> >> */ >> >> >> host->use_ecc = false; >> >> >> + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> >> > > Are we sure there is no layout with only 1 step? >> >> >> All the layouts are such that, the BBM will come in >> >> first byte of spare area. >> >> >> For 4 bit ECC, the cw_size is 528 so for 2K page >> >> >> 2048 - 528 * 3 = 464 >> > > My question was more about small page NANDs. But I suppose it works >> > too if ecc->steps == 1. >> > >> Correct Miquel. >> >> >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in >> >> full page. >> >> >> > >> >> clear_bam_transaction(nandc); >> >> >> - ret = copy_last_cw(host, page); >> >> >> - if (ret) >> >> >> + clear_read_regs(nandc); >> >> >> + >> >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); >> >> >> + update_rw_regs(host, 1, true); >> >> >> + >> >> >> + /* >> >> >> + * The last codeword data will be copied from NAND device to NAND >> >> >> + * controller internal HW buffer. Copy only required BBM size bytes >> >> >> + * from this HW buffer to bbm_bytes_buf which is present at >> >> >> + * bbpos offset. >> >> >> + */ >> >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> + config_nand_single_cw_page_read(nandc); >> >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, >> >> >> + host->bbm_size, 0); >> >> >> + >> >> >> + ret = submit_descs(nandc); >> >> >> + free_descs(nandc); >> >> >> + if (ret) { >> >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); >> >> >> goto err; >> >> >> + } >> >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); >> >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> >> goto err; >> >> >> } >> >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> >> >> - >> >> >> - bad = nandc->data_buffer[bbpos] != 0xff; >> >> >> + bad = bbm_bytes_buf[0] != 0xff; >> >> > > This is suspect as it still points to the beginning of the data buffer. >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? >> >> > >> >> The main thing here is >> >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> After reading one complete CW from NAND, the data will be still >> >> in NAND HW buffer. >> >> >> The above register tells that we need to read data from >> >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh >> >> and 2 byte for 16 bus width) in bbm_bytes_buf. >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then >> > it's ok. >> > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. >> >> and bbm_bytes_buf[1] will contain the BBM second byte. >> >> >> Regards, >> >> Abhishek >> >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) >> >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); >> > > Sorry, my mistake, I did not see the above line. >> > > However, technically, the BBM could be located in the first, second or >> > last page of the block. You should check the three of them are 0xFF >> > before declaring the block is not bad. >> > > The more I look at the function, the more I wonder if you actually need >> > it. Why does the generic nand_block_bad() implementation in the core >> > do not fit? >> >> The BBM bytes can be accessed in raw mode only for QCOM NAND >> Contoller. We started with following patch for initial patches >> >> http://patchwork.ozlabs.org/patch/508565/ >> >> I am also not very much sure, how can we go ahead now. >> Ideally we need to use generic function only which >> requires raw_read. >> > > I see, thanks for pointing this thread. > > Well for now then let's keep our driver-specific implementation. > > I will just ask you to do a consistent check as requested above (you > can copy code from the core) and add a comment above this function > explaining why it is needed (what you just told me). > Hi Miquel, I explored more regarding making custom bad block functions in this thread and it looks like, we can move to generic block_bad function by small changes in QCOM NAND driver only. The main problem was, in read page with ECC, the bad block byte was skipped. But controller is copying the bad block bytes in another register with following status bytes. BAD_BLOCK_STATUS : With every page read operation, when the controller reads a page with a bad block, it writes the bad block status data into this register. We can update the BBM bytes at start of OOB data in read_oob function with these status bytes. It will help in getting rid of driver-specific implementation for chip->block_bad. For chip->block_markbad, if we want to get rid of driver-specific implementation then we can have following logic in write_oob function check for bad block bytes in oob and do the raw write for updating BBM bytes alone in flash if BBM bytes are non 0xff. Thanks, Abhishek
Hi Abhishek, Boris, one question for you below :) > >> >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in > >> >> full page. > >> >> >> > >> >> clear_bam_transaction(nandc); > >> >> >> - ret = copy_last_cw(host, page); > >> >> >> - if (ret) > >> >> >> + clear_read_regs(nandc); > >> >> >> + > >> >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); > >> >> >> + update_rw_regs(host, 1, true); > >> >> >> + > >> >> >> + /* > >> >> >> + * The last codeword data will be copied from NAND device to NAND > >> >> >> + * controller internal HW buffer. Copy only required BBM size bytes > >> >> >> + * from this HW buffer to bbm_bytes_buf which is present at > >> >> >> + * bbpos offset. > >> >> >> + */ > >> >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > >> >> >> + config_nand_single_cw_page_read(nandc); > >> >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, > >> >> >> + host->bbm_size, 0); > >> >> >> + > >> >> >> + ret = submit_descs(nandc); > >> >> >> + free_descs(nandc); > >> >> >> + if (ret) { > >> >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); > >> >> >> goto err; > >> >> >> + } > >> >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); > >> >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) > >> >> >> goto err; > >> >> >> } > >> >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); > >> >> >> - > >> >> >> - bad = nandc->data_buffer[bbpos] != 0xff; > >> >> >> + bad = bbm_bytes_buf[0] != 0xff; > >> >> > > This is suspect as it still points to the beginning of the data buffer. > >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? > >> >> > > >> >> The main thing here is > >> >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); > >> >> >> After reading one complete CW from NAND, the data will be still > >> >> in NAND HW buffer. > >> >> >> The above register tells that we need to read data from > >> >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh > >> >> and 2 byte for 16 bus width) in bbm_bytes_buf. > >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then > >> > it's ok. > >> > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. > >> >> and bbm_bytes_buf[1] will contain the BBM second byte. > >> >> >> Regards, > >> >> Abhishek > >> >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) > >> >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); > >> >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); > >> > > Sorry, my mistake, I did not see the above line. > >> > > However, technically, the BBM could be located in the first, second or > >> > last page of the block. You should check the three of them are 0xFF > >> > before declaring the block is not bad. > >> > > The more I look at the function, the more I wonder if you actually need > >> > it. Why does the generic nand_block_bad() implementation in the core > >> > do not fit? > >> >> The BBM bytes can be accessed in raw mode only for QCOM NAND > >> Contoller. We started with following patch for initial patches > >> >> http://patchwork.ozlabs.org/patch/508565/ > >> >> I am also not very much sure, how can we go ahead now. > >> Ideally we need to use generic function only which > >> requires raw_read. > >> > > I see, thanks for pointing this thread. > > > Well for now then let's keep our driver-specific implementation. > > > I will just ask you to do a consistent check as requested above (you > > can copy code from the core) and add a comment above this function > > explaining why it is needed (what you just told me). > > > Hi Miquel, > > I explored more regarding making custom bad block functions in this > thread and it looks like, we can move to generic block_bad function > by small changes in QCOM NAND driver > only. The main problem was, in read page with ECC, the bad block > byte was skipped. > > But controller is copying the bad block bytes in another register > with following status bytes. > > BAD_BLOCK_STATUS : With every page read operation, when the controller > reads a page with a bad block, it writes the bad block status data into > this register. > > We can update the BBM bytes at start of OOB data in read_oob function > with these status bytes. It will help in getting rid of driver-specific > implementation for chip->block_bad. If think this is acceptable. > > For chip->block_markbad, if we want to get rid of > driver-specific implementation then we can have > following logic > > in write_oob function check for bad block bytes in oob > and do the raw write for updating BBM bytes alone in > flash if BBM bytes are non 0xff. Ok but this will have to be properly explained in a descriptive comment! Maybe Boris can give its point of view on the subject. Is it worth adding the above 'hacks' in the qcom driver and get rid of the driver-specific ->is_bad()/->mark_bad() impementations? Thanks, Miquèl
On 2018-06-18 17:05, Miquel Raynal wrote: > Hi Abhishek, > > Boris, one question for you below :) > >> >> >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in >> >> >> full page. >> >> >> >> > >> >> clear_bam_transaction(nandc); >> >> >> >> - ret = copy_last_cw(host, page); >> >> >> >> - if (ret) >> >> >> >> + clear_read_regs(nandc); >> >> >> >> + >> >> >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); >> >> >> >> + update_rw_regs(host, 1, true); >> >> >> >> + >> >> >> >> + /* >> >> >> >> + * The last codeword data will be copied from NAND device to NAND >> >> >> >> + * controller internal HW buffer. Copy only required BBM size bytes >> >> >> >> + * from this HW buffer to bbm_bytes_buf which is present at >> >> >> >> + * bbpos offset. >> >> >> >> + */ >> >> >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> >> + config_nand_single_cw_page_read(nandc); >> >> >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, >> >> >> >> + host->bbm_size, 0); >> >> >> >> + >> >> >> >> + ret = submit_descs(nandc); >> >> >> >> + free_descs(nandc); >> >> >> >> + if (ret) { >> >> >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); >> >> >> >> goto err; >> >> >> >> + } >> >> >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); >> >> >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> >> >> goto err; >> >> >> >> } >> >> >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> >> >> >> - >> >> >> >> - bad = nandc->data_buffer[bbpos] != 0xff; >> >> >> >> + bad = bbm_bytes_buf[0] != 0xff; >> >> >> > > This is suspect as it still points to the beginning of the data buffer. >> >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? >> >> >> > >> >> >> The main thing here is >> >> >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> >> After reading one complete CW from NAND, the data will be still >> >> >> in NAND HW buffer. >> >> >> >> The above register tells that we need to read data from >> >> >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh >> >> >> and 2 byte for 16 bus width) in bbm_bytes_buf. >> >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then >> >> > it's ok. >> >> > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. >> >> >> and bbm_bytes_buf[1] will contain the BBM second byte. >> >> >> >> Regards, >> >> >> Abhishek >> >> >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) >> >> >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> >> >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); >> >> > > Sorry, my mistake, I did not see the above line. >> >> > > However, technically, the BBM could be located in the first, second or >> >> > last page of the block. You should check the three of them are 0xFF >> >> > before declaring the block is not bad. >> >> > > The more I look at the function, the more I wonder if you actually need >> >> > it. Why does the generic nand_block_bad() implementation in the core >> >> > do not fit? >> >> >> The BBM bytes can be accessed in raw mode only for QCOM NAND >> >> Contoller. We started with following patch for initial patches >> >> >> http://patchwork.ozlabs.org/patch/508565/ >> >> >> I am also not very much sure, how can we go ahead now. >> >> Ideally we need to use generic function only which >> >> requires raw_read. >> >> > > I see, thanks for pointing this thread. >> > > Well for now then let's keep our driver-specific implementation. >> > > I will just ask you to do a consistent check as requested above (you >> > can copy code from the core) and add a comment above this function >> > explaining why it is needed (what you just told me). >> > >> Hi Miquel, >> >> I explored more regarding making custom bad block functions in this >> thread and it looks like, we can move to generic block_bad function >> by small changes in QCOM NAND driver >> only. The main problem was, in read page with ECC, the bad block >> byte was skipped. >> >> But controller is copying the bad block bytes in another register >> with following status bytes. >> >> BAD_BLOCK_STATUS : With every page read operation, when the >> controller >> reads a page with a bad block, it writes the bad block status data >> into >> this register. >> >> We can update the BBM bytes at start of OOB data in read_oob >> function >> with these status bytes. It will help in getting rid of >> driver-specific >> implementation for chip->block_bad. > > If think this is acceptable. > >> >> For chip->block_markbad, if we want to get rid of >> driver-specific implementation then we can have >> following logic >> >> in write_oob function check for bad block bytes in oob >> and do the raw write for updating BBM bytes alone in >> flash if BBM bytes are non 0xff. > > Ok but this will have to be properly explained in a descriptive > comment! > > Maybe Boris can give its point of view on the subject. Is it worth > adding the above 'hacks' in the qcom driver and get rid of the > driver-specific ->is_bad()/->mark_bad() impementations? Thanks Miquel. I will remove this patch from this patch series and will send separate patch series for all bad block handling related changes once things are finalized. Regards, Abhishek
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index d693b5f..f72bc8a 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -1769,41 +1769,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf, return parse_read_errors(host, data_buf_start, oob_buf_start); } -/* - * a helper that copies the last step/codeword of a page (containing free oob) - * into our local buffer - */ -static int copy_last_cw(struct qcom_nand_host *host, int page) -{ - struct nand_chip *chip = &host->chip; - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); - struct nand_ecc_ctrl *ecc = &chip->ecc; - int size; - int ret; - - clear_read_regs(nandc); - - size = host->use_ecc ? host->cw_data : host->cw_size; - - /* prepare a clean read buffer */ - memset(nandc->data_buffer, 0xff, size); - - set_address(host, host->cw_size * (ecc->steps - 1), page); - update_rw_regs(host, 1, true); - - config_nand_single_cw_page_read(nandc); - - read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0); - - ret = submit_descs(nandc); - if (ret) - dev_err(nandc->dev, "failed to copy last codeword\n"); - - free_descs(nandc); - - return ret; -} - /* implements ecc->read_page() */ static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) @@ -2118,6 +2083,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) struct nand_ecc_ctrl *ecc = &chip->ecc; int page, ret, bbpos, bad = 0; u32 flash_status; + u8 *bbm_bytes_buf = chip->data_buf; page = (int)(ofs >> chip->page_shift) & chip->pagemask; @@ -2128,11 +2094,31 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) * that contains the BBM */ host->use_ecc = false; + bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); clear_bam_transaction(nandc); - ret = copy_last_cw(host, page); - if (ret) + clear_read_regs(nandc); + + set_address(host, host->cw_size * (ecc->steps - 1), page); + update_rw_regs(host, 1, true); + + /* + * The last codeword data will be copied from NAND device to NAND + * controller internal HW buffer. Copy only required BBM size bytes + * from this HW buffer to bbm_bytes_buf which is present at + * bbpos offset. + */ + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); + config_nand_single_cw_page_read(nandc); + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, + host->bbm_size, 0); + + ret = submit_descs(nandc); + free_descs(nandc); + if (ret) { + dev_err(nandc->dev, "failed to copy bad block bytes\n"); goto err; + } flash_status = le32_to_cpu(nandc->reg_read_buf[0]); @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) goto err; } - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); - - bad = nandc->data_buffer[bbpos] != 0xff; + bad = bbm_bytes_buf[0] != 0xff; if (chip->options & NAND_BUSWIDTH_16) - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); + bad = bad || (bbm_bytes_buf[1] != 0xff); err: return bad; }
The QCOM NAND controller layout is such that, the bad block byte offset for last codeword will come to first byte in spare area. Currently, the raw read for last codeword is being done with copy_last_cw function. It does following 2 things: 1. Read the last codeword bytes from NAND chip to NAND controller internal HW buffer. 2. Copy all these bytes from HW buffer to actual buffer. For bad block check, maximum two bytes are required so instead of copying the complete bytes in step 2, only those bbm_size bytes can be copied. This patch does minor code reorganization for the same. After this, copy_last_cw function won’t be required. Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> --- * Changes from v2: 1. Changed commit message and comments slightly * Changes from v1: NEW CHANGE drivers/mtd/nand/raw/qcom_nandc.c | 66 +++++++++++++++------------------------ 1 file changed, 25 insertions(+), 41 deletions(-)