Messages in this thread | | | Date | Tue, 27 Jan 2015 09:26:09 +0530 | From | Archit Taneja <> | Subject | Re: [PATCH 2/5] mtd: nand: Add qcom nand controller driver |
| |
On 01/27/2015 02:35 AM, Kevin Cernekee wrote: > On Wed, Jan 21, 2015 at 10:36 PM, Archit Taneja <architt@codeaurora.org> wrote: >> On 01/21/2015 06:24 AM, Daniel Ehrenberg wrote: >>> On Fri, Jan 16, 2015 at 6:48 AM, Archit Taneja <architt@codeaurora.org> >>> wrote: >>>> >>>> +/* >>>> + * the bad block marker is readable only when we read the page with ECC >>>> + * disabled. all the read/write commands like NAND_CMD_READOOB, >>>> NAND_CMD_READ0 >>>> + * and NAND_CMD_PAGEPROG are executed in the driver with ECC enabled. >>>> therefore, >>>> + * the default nand helper functions to detect a bad block or mark a bad >>>> block >>>> + * can't be used. >>>> + */ >>>> +static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs, int >>>> getchip) >>>> +{ >>>> + int page, r, mark, bad = 0; >>>> + struct nand_chip *chip = mtd->priv; >>>> + struct nand_ecc_ctrl *ecc = &chip->ecc; >>>> + int cwperpage = ecc->steps; >>>> + struct qcom_nandc_data *this = chip->priv; >>>> + u32 flash_status; >>>> + >>>> + pre_command(this, NAND_CMD_NONE); >>>> + >>>> + page = (int)(ofs >> chip->page_shift) & chip->pagemask; >>>> + >>>> + /* >>>> + * configure registers for a raw page read, the address is set to >>>> the >>>> + * beginning of the last codeword >>>> + */ >>>> + this->use_ecc = false; >>>> + set_address(this, this->cw_size * (cwperpage - 1), page); >>>> + >>>> + /* we just read one codeword that contains the bad block marker >>>> */ >>>> + update_rw_regs(this, 1, true); >>>> + >>>> + read_cw(this, this->cw_size, 0); >>>> + >>>> + r = submit_descs(this); >>>> + if (r) { >>>> + dev_err(this->dev, "error submitting descs\n"); >>>> + goto err; >>>> + } >>>> + >>>> + flash_status = this->reg_read_buf[0]; >>>> + >>>> + /* >>>> + * unable to read the marker successfully, is that sufficient to >>>> report >>>> + * the block as bad? >>>> + */ >>>> + if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) { >>>> + dev_warn(this->dev, "error while reading bad block >>>> mark\n"); >>>> + goto err; >>>> + } >>>> + >>>> + mark = mtd->writesize - (this->cw_size * (cwperpage - 1)); >>>> + >>>> + if (chip->options & NAND_BUSWIDTH_16) >>>> + bad = this->data_buffer[mark] != 0xff || >>>> + this->data_buffer[mark + 1] != 0xff; >>>> + >>>> + bad = this->data_buffer[mark] != 0xff; >>>> +err: >>>> + free_descs(this); >>>> + >>>> + return bad; >>>> +} >>>> + >>>> +static int qcom_nandc_block_markbad(struct mtd_info *mtd, loff_t ofs) >>>> +{ >>>> + int page, r; >>>> + struct nand_chip *chip = mtd->priv; >>>> + struct nand_ecc_ctrl *ecc = &chip->ecc; >>>> + int cwperpage = ecc->steps; >>>> + struct qcom_nandc_data *this = chip->priv; >>>> + u32 flash_status; >>>> + >>>> + pre_command(this, NAND_CMD_NONE); >>>> + >>>> + /* fill our internal buffer's oob area with 0's */ >>>> + memset(this->data_buffer, 0x00, mtd->writesize + mtd->oobsize); >>>> + >>>> + page = (int)(ofs >> chip->page_shift) & chip->pagemask; >>>> + >>>> + this->use_ecc = false; >>>> + set_address(this, this->cw_size * (cwperpage - 1), page); >>>> + >>>> + /* we just write to one codeword that contains the bad block >>>> marker*/ >>>> + update_rw_regs(this, 1, false); >>>> + >>>> + /* >>>> + * overwrite the last codeword with 0s, this will result in >>>> setting >>>> + * the bad block marker to 0 too >>>> + */ >>>> + write_cw(this, this->cw_size, 0); >>>> + >>>> + r = submit_descs(this); >>>> + if (r) { >>>> + dev_err(this->dev, "error submitting descs\n"); >>>> + r = -EIO; >>>> + goto err; >>>> + } >>>> + >>>> + flash_status = this->reg_read_buf[0]; >>>> + >>>> + if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) >>>> + r = -EIO; >>>> + >>>> +err: >>>> + free_descs(this); >>>> + >>>> + return r; >>>> +} > > Would it be possible to refactor this code so that it implements > generic read_oob_raw() and write_oob_raw() callbacks, which can > read/write arbitrary uncorrected OOB bytes instead of just the BBI? > Or is the controller limited in which OOB bytes can be directly > accessed?
When accessing the page with ECC disabled, there doesn't seem to be any limitation.
We have 528/32 bytes per codeword, and we can access all of it in raw mode.
> > One advantage of implementing the generic raw read/write callbacks is > that the MTD subsystem already has logic to handle old NAND chips with > different BBI positions, bitflips in the BBI, and other corner cases. > >>> Looks like this code marks block bad and reads bad block information >>> based on information in the OOB area. And in qcom_nandc_init, >>> NAND_SKIP_BBTSCAN is set, meaning that this is the code used in >>> practice on the chip in the mtd_block_isbad path. Can this driver be >>> used with an on-flash OOB table by editing the init function's chip >>> flags, or would it need more significant changes to allow that? >> >> >> The code doesn't exactly read the OOB area. When the ECC HW block is >> enabled, the bad block isn't in either oob or data area! We can access it >> only when ECC is disabled. With ECC disabled, the bad block marker lies in >> the last codeword somewhere in the middle of the user data. For the >> mtd_read_oob()/write_oob() functions, we have the ECC always enabled, hence, >> we never access the bad block marker through them at all. >> >> Creating an on-flash bad block table won't work right now. The reason is >> that the nand_bbt library assumes that it can find the bad block marker by >> reading oob. While creating a bbt in memory, it scans the device for bad >> blocks using the function scan_block_fast(). This would currently result in >> not reading the bad block marker, and therefore break things. >> >> I'm trying to find out if there is a way by which the controller can access >> the bad block marker with ECC HW enabled. If that works, we can use the >> nand_bbt helper as is. For now, I wanted to get the driver upstream without >> the BBT functionality. > > So, back in commit a7e68834fc2739 ("mtd: nand: use ECC, if present, > when scanning OOB"), the BBT code was changed to use MTD_OPS_PLACE_OOB > instead of MTD_OPS_RAW, because some controllers offer the ability to > provide corrected OOB data and we wanted to use it if possible. The > changelog notes that many existing drivers still disabled ECC when > reading OOB in MTD_OPS_PLACE_OOB mode. > > Now it sounds like the qcom_nandc controller can either provide access > to some corrected OOB bytes (not including the BBI) when > MTD_OPS_PLACE_OOB is used, or all (?) uncorrected OOB bytes (including > the BBI) when MTD_OPS_RAW is used.
That's right.
> > Two possible options are: > > 1) qcom_nandc should just disable ECC when performing OOB reads/writes > in MTD_OPS_PLACE_OOB mode
What prevented me from doing this was how the chip->ecc.read_page/write_page funcs are defined. They take the param called oob_required.
If oob_required is set, we would need to read/write page contents with ECC disabled too. Or, perform 2 rounds of acceses. One for the page contents with ECC enabled, and other for oob with ECC disabled.
I don't know how often oob_required is set by the mtd subsystem, if it's a path that is never exercised, we could try this option.
> > 2) MTD should be made aware of whether the controller can access the > BBI in MTD_OPS_PLACE_OOB mode, and fall back to MTD_OPS_RAW mode if > the hardware cannot do so
That sounds like a good option. A NAND chip option mentioning this capability could be helpful here.
Thanks, Archit
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |