Re: [PATCH 8/9] mtd: nand: qcom: helper function for raw read

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

 



On 2018-04-22 21:49, Miquel Raynal wrote:
Hi Abhishek,

On Thu, 12 Apr 2018 12:36:42 +0530, Abhishek Sahu
<absahu@xxxxxxxxxxxxxx> wrote:

On 2018-04-10 15:14, Miquel Raynal wrote:
> Hi Abhishek,
> > On Wed,  4 Apr 2018 18:12:24 +0530, Abhishek Sahu
> <absahu@xxxxxxxxxxxxxx> wrote:
> >> This patch does minor code reorganization for raw reads.
>> Currently the raw read is required for complete page but for
>> subsequent patches related with erased codeword bit flips
>> detection, only few CW should be read. So, this patch adds
>> helper function and introduces the read CW bitmask which
>> specifies which CW reads are required in complete page.
> > I am not sure this is the right approach for subpage reads. If the
> controller supports it, you should just enable it in chip->options.
>
  Thanks Miquel.

  It is internal to this file only. This patch makes one static helper
  function which provides the support to read subpages.

Drivers should expose raw helpers, why keeping this helper static then?


>> >> Signed-off-by: Abhishek Sahu <absahu@xxxxxxxxxxxxxx>
>> ---
>>  drivers/mtd/nand/qcom_nandc.c | 186 >> +++++++++++++++++++++++++-----------------
>>  1 file changed, 110 insertions(+), 76 deletions(-)
>> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c
>> index 40c790e..f5d1fa4 100644
>> --- a/drivers/mtd/nand/qcom_nandc.c
>> +++ b/drivers/mtd/nand/qcom_nandc.c
>> @@ -1590,6 +1590,114 @@ static int check_flash_errors(struct >> qcom_nand_host *host, int cw_cnt)
>>  }
>> >>  /*
>> + * Helper to perform the page raw read operation. The read_cw_mask >> will be
>> + * used to specify the codewords for which the data should be read. >> The
>> + * single page contains multiple CW. Sometime, only few CW data is >> required
>> + * in complete page. Also, start address will be determined with
>> + * this CW mask to skip unnecessary data copy from NAND flash device. >> Then,
>> + * actual data copy from NAND controller to data buffer will be done >> only
>> + * for the CWs which have the mask set.
>> + */
>> +static int
>> +nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>> +		    u8 *data_buf, u8 *oob_buf,
>> +		    int page, unsigned long read_cw_mask)
>> +{
>> +	struct qcom_nand_host *host = to_qcom_nand_host(chip);
>> +	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	int i, ret;
>> +	int read_loc, start_step, last_step;
>> +
>> +	nand_read_page_op(chip, page, 0, NULL, 0);
>> +
>> +	host->use_ecc = false;
>> +	start_step = ffs(read_cw_mask) - 1;
>> +	last_step = fls(read_cw_mask);
>> +
>> +	clear_bam_transaction(nandc);
>> +	set_address(host, host->cw_size * start_step, page);
>> +	update_rw_regs(host, last_step - start_step, true);
>> +	config_nand_page_read(nandc);
>> +
>> +	for (i = start_step; i < last_step; i++) {
>> +		int data_size1, data_size2, oob_size1, oob_size2;
>> +		int reg_off = FLASH_BUF_ACC;
>> +
>> +		data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
>> +		oob_size1 = host->bbm_size;
>> +
>> +		if (i == (ecc->steps - 1)) {
>> +			data_size2 = ecc->size - data_size1 -
>> +				     ((ecc->steps - 1) << 2);
>> +			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
>> +				    host->spare_bytes;
>> +		} else {
>> +			data_size2 = host->cw_data - data_size1;
>> +			oob_size2 = host->ecc_bytes_hw + host->spare_bytes;
>> +		}
>> +
>> +		/*
>> +		 * Don't perform actual data copy from NAND controller to data
>> +		 * buffer through DMA for this codeword
>> +		 */
>> +		if (!(read_cw_mask & BIT(i))) {
>> +			if (nandc->props->is_bam)
>> +				nandc_set_read_loc(nandc, 0, 0, 0, 1);
>> +
>> +			config_nand_cw_read(nandc, false);
>> +
>> +			data_buf += data_size1 + data_size2;
>> +			oob_buf += oob_size1 + oob_size2;
>> +
>> +			continue;
>> +		}
>> +
>> +		if (nandc->props->is_bam) {
>> +			read_loc = 0;
>> +			nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
>> +			read_loc += data_size1;
>> +
>> +			nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
>> +			read_loc += oob_size1;
>> +
>> +			nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
>> +			read_loc += data_size2;
>> +
>> +			nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
>> +		}
>> +
>> +		config_nand_cw_read(nandc, false);
>> +
>> +		read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
>> +		reg_off += data_size1;
>> +		data_buf += data_size1;
>> +
>> +		read_data_dma(nandc, reg_off, oob_buf, oob_size1, 0);
>> +		reg_off += oob_size1;
>> +		oob_buf += oob_size1;
>> +
>> +		read_data_dma(nandc, reg_off, data_buf, data_size2, 0);
>> +		reg_off += data_size2;
>> +		data_buf += data_size2;
>> +
>> +		read_data_dma(nandc, reg_off, oob_buf, oob_size2, 0);
>> +		oob_buf += oob_size2;
>> +	}
>> +
>> +	ret = submit_descs(nandc);
>> +	if (ret)
>> +		dev_err(nandc->dev, "failure to read raw page\n");
>> +
>> +	free_descs(nandc);
>> +
>> +	if (!ret)
>> +		ret = check_flash_errors(host, last_step - start_step);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>>   * reads back status registers set by the controller to notify page >> read
>>   * errors. this is equivalent to what 'ecc->correct()' would do.
>>   */
>> @@ -1839,82 +1947,8 @@ static int qcom_nandc_read_page_raw(struct >> mtd_info *mtd,
>>  				    struct nand_chip *chip, uint8_t *buf,
>>  				    int oob_required, int page)
>>  {
>> -	struct qcom_nand_host *host = to_qcom_nand_host(chip);
>> -	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> -	u8 *data_buf, *oob_buf;
>> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> -	int i, ret;
>> -	int read_loc;
>> -
>> -	nand_read_page_op(chip, page, 0, NULL, 0);
>> -	data_buf = buf;
>> -	oob_buf = chip->oob_poi;
>> -
>> -	host->use_ecc = false;
>> -
>> -	clear_bam_transaction(nandc);
>> -	update_rw_regs(host, ecc->steps, true);
>> -	config_nand_page_read(nandc);
>> -
>> -	for (i = 0; i < ecc->steps; i++) {
>> -		int data_size1, data_size2, oob_size1, oob_size2;
>> -		int reg_off = FLASH_BUF_ACC;
>> -
>> -		data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
>> -		oob_size1 = host->bbm_size;
>> -
>> -		if (i == (ecc->steps - 1)) {
>> -			data_size2 = ecc->size - data_size1 -
>> -				     ((ecc->steps - 1) << 2);
>> -			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
>> -				    host->spare_bytes;
>> -		} else {
>> -			data_size2 = host->cw_data - data_size1;
>> -			oob_size2 = host->ecc_bytes_hw + host->spare_bytes;
>> -		}
>> -
>> -		if (nandc->props->is_bam) {
>> -			read_loc = 0;
>> -			nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
>> -			read_loc += data_size1;
>> -
>> -			nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
>> -			read_loc += oob_size1;
>> -
>> -			nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
>> -			read_loc += data_size2;
>> -
>> -			nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
>> -		}
>> -
>> -		config_nand_cw_read(nandc, false);
>> -
>> -		read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
>> -		reg_off += data_size1;
>> -		data_buf += data_size1;
>> -
>> -		read_data_dma(nandc, reg_off, oob_buf, oob_size1, 0);
>> -		reg_off += oob_size1;
>> -		oob_buf += oob_size1;
>> -
>> -		read_data_dma(nandc, reg_off, data_buf, data_size2, 0);
>> -		reg_off += data_size2;
>> -		data_buf += data_size2;
>> -
>> -		read_data_dma(nandc, reg_off, oob_buf, oob_size2, 0);
>> -		oob_buf += oob_size2;
>> -	}
>> -
>> -	ret = submit_descs(nandc);
>> -	if (ret)
>> -		dev_err(nandc->dev, "failure to read raw page\n");
>> -
>> -	free_descs(nandc);
>> -
>> -	if (!ret)
>> -		ret = check_flash_errors(host, ecc->steps);
>> -
>> -	return 0;
>> +	return nandc_read_page_raw(mtd, chip, buf, chip->oob_poi, page,
>> +				   BIT(chip->ecc.steps) - 1);
> > I don't understand this. chip->ecc.steps is constant, right? So you
> always ask for the same subpage?

  We need to do raw read for subpages in which we got uncorrectable
error in next patch for erased page bitflip detection. This patch does
  reorganization of raw read and moves common code in helper function
  nandc_read_page_raw.

  For nomral raw read, all the subpages will be read so
  BIT(chip->ecc.steps) - 1 is used for qcom_nandc_read_page_raw.

  While for erased page raw read, only those sub pages will be
  read for which the controller gives the uncorrectable error.

Still not okay: the driver should expose a way to do raw reads no
matter the length and the start and you should use that in a generic
way.


 Thanks Miquel.
 I will explore regarding that.
 Looks like, we can some helper like read_subpage.

 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