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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux