Re: [PATCH 2/5] mtd: nand: Add qcom nand controller driver

[Archit Taneja <architt@codeaurora.org>]

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