by Miquel Raynal

[permalink] [raw]

Subject: Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

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

2018-06-20 09:32:06

by Abhishek Sahu

[permalink] [raw]

Subject: Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

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