On Wed, Jan 21, 2015 at 10:36 PM, Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
> On 01/21/2015 06:24 AM, Daniel Ehrenberg wrote:
>> On Fri, Jan 16, 2015 at 6:48 AM, Archit Taneja <architt@xxxxxxxxxxxxxx>
>> 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?
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.
Two possible options are:
1) qcom_nandc should just disable ECC when performing OOB reads/writes
in MTD_OPS_PLACE_OOB mode
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
Any thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
