Re: Re: [PATCH v13 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others

[赵仪峰 <yifeng.zhao@rock-chips.com>]

Hi Johan,

On 2020/10/31 19:58, Johan Jonker wrote:
>Hi Yifeng,
...
>> +
>> +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *buf, int oob_on,
>> +	  int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rknand = rk_nfc_to_rknand(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
>> +	NFC_MIN_OOB_PER_STEP;
>> +	int pages_per_blk = mtd->erasesize / mtd->writesize;
>> +	dma_addr_t dma_data, dma_oob;
>
>> +	int ret = 0, i, boot_rom_mode = 0;
>
>	int ret = 0, i, cnt, boot_rom_mode = 0;
>
>> +	int bitflips = 0, bch_st;
>> +	u8 *oob;
>> +	u32 tmp;
>> +
>> +	nand_read_page_op(chip, page, 0, NULL, 0);
>> +
>> +	dma_data = dma_map_single(nfc->dev, nfc->page_buf,
>> +	  mtd->writesize,
>> +	  DMA_FROM_DEVICE);
>> +	dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
>> +	ecc->steps * oob_step,
>> +	DMA_FROM_DEVICE);
>> +
>> +	/*
>> +	* The first blocks (4, 8 or 16 depending on the device)
>> +	* are used by the boot ROM.
>> +	* Configure the ECC algorithm supported by the boot ROM.
>> +	*/
>
>> +	if ((page < pages_per_blk * rknand->boot_blks) &&
>
>> +	if ((page < (pages_per_blk * rknand->boot_blks)) &&
>
>> +	    (chip->options & NAND_IS_BOOT_MEDIUM)) {
>> +	boot_rom_mode = 1;
>> +	if (rknand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc,
>> +	    rknand->boot_ecc);
>> +	}
>> +
>> +	reinit_completion(&nfc->done);
>> +	writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
>> +	rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
>> +	  dma_oob);
>> +	ret = wait_for_completion_timeout(&nfc->done,
>> +	  msecs_to_jiffies(100));
>> +	if (!ret)
>> +	dev_warn(nfc->dev, "read: wait dma done timeout.\n");
>> +	/*
>> +	* Whether the DMA transfer is completed or not. The driver
>> +	* needs to check the NFC`s status register to see if the data
>> +	* transfer was completed.
>> +	*/
>> +	ret = rk_nfc_wait_for_xfer_done(nfc);
>
>add empty line
>
>> +	dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
>> +	DMA_FROM_DEVICE);
>> +	dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
>> +	DMA_FROM_DEVICE);
>> +
>> +	if (ret) {
>
>> +	bitflips = -EIO;
>
>	ret = -EIO;
>
>return only "0" or official error codes
>
>> +	dev_err(nfc->dev,
>> +	"read: wait transfer done timeout.\n");
>> +	goto out;
>> +	}
>> +
>> +	for (i = 1; i < ecc->steps; i++) {
>> +	oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
>> +	if (nfc->cfg->type == NFC_V9)
>> +	tmp = nfc->oob_buf[i];
>> +	else
>> +	tmp = nfc->oob_buf[i * (oob_step / 4)];
>> +	*oob++ = (u8)tmp;
>> +	*oob++ = (u8)(tmp >> 8);
>> +	*oob++ = (u8)(tmp >> 16);
>> +	*oob++ = (u8)(tmp >> 24);
>> +	}
>> +
>> +	for (i = 0; i < (ecc->steps / 2); i++) {
>> +	bch_st = readl_relaxed(nfc->regs +
>> +	       nfc->cfg->bch_st_off + i * 4);
>> +	if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
>> +	    bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
>> +	mtd->ecc_stats.failed++;
>
>> +	bitflips = 0;
>
>max_bitflips = -1;
>
>use max_bitflips only for the error warning, not as return value
>
>> +	} else {
>
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>
>use ret only with "0" or official error codes, use cnt instead
>
>	cnt = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>
>> +	mtd->ecc_stats.corrected += ret;
>	mtd->ecc_stats.corrected += cnt;
>
>> +	bitflips = max_t(u32, bitflips, ret);
>
>	bitflips = max_t(u32, bitflips, cnt);
>
>> +
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>
>	cnt = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>
>> +	mtd->ecc_stats.corrected += ret;
>
>	mtd->ecc_stats.corrected += cnt;
>
>> +	bitflips = max_t(u32, bitflips, ret);
>
>	bitflips = max_t(u32, bitflips, cnt);
>> +	}
>> +	}
>> +out:
>> +	memcpy(buf, nfc->page_buf, mtd->writesize);
>> +
>
>> +	if (boot_rom_mode && rknand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>> +
>
>> +	if (bitflips > ecc->strength)
>
>	if (bitflips  == -1) {
>	ret = -EIO;
>
>> +	dev_err(nfc->dev, "read page: %x ecc error!\n", page);
>
>}

It cannot return - EIO while ECC fail, refer to the function nand_do_read_ops,

maybe need do read_retry while ECC fail.

I think return 0 is better while ecc fail，as suggested by Miquel.

In other cases, return the actual ECC error bit, because the file system(such as ubifs) needs to

know whether the data is reliable or needs to be refreshed (-EUCLEAN).

int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
{

...

return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;

}

>> +
>> +	/*
>> +	* Deselect the currently selected target after the ops is done
>> +	* to reduce the power consumption.
>> +	*/
>> +	rk_nfc_select_chip(chip, -1);
>> +
>
>> +	return bitflips;
>
>	return ret;
>
>Return only "0" or official error codes
>If you want to do a "bad block scan" function in user space analyse/use
>"mtd->ecc_stats" instead.
>
>> +}

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/