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

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

Hi Johan,

void nand_deselect_target(struct nand_chip *chip)
{
	if (chip->legacy.select_chip)
		chip->legacy.select_chip(chip, -1);

	chip->cur_cs = -1;
}

I need add the code below and it work. 

   chip->legacy.select_chip = rk_nfc_select_chip;

But I found almost all nandc drivers do not add this code. Is there any other way to implement it？

--------------

>Hi Yifeng,
>
>In some functions you deselect the chips.
>The MTD frame work has a functions for that and also keeps track of its
>select status, so I think that you shouldn't poke with that yourself and
>should therefore remove the deselections from your driver.
>
>/**
> * nand_deselect_target() - Deselect the currently selected target
> * @chip: NAND chip object
> *
> * Deselect the currently selected NAND target. The result of operations
> * executed on @chip after the target has been deselected is undefined.
> */
>void nand_deselect_target(struct nand_chip *chip)
>{
>	if (chip->legacy.select_chip)
>	chip->legacy.select_chip(chip, -1);
>
>	chip->cur_cs = -1;
>}
>EXPORT_SYMBOL_GPL(nand_deselect_target);
>
>
>On 10/31/20 12:58 PM, Johan Jonker wrote:
>
>[..]
>
>> On 10/28/20 10:53 AM, Yifeng Zhao wrote:
>
>[..]
>
>>> +
>>> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const 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 nand_ecc_ctrl *ecc = &chip->ecc;
>>> +	int ret = 0;
>>> +	u32 i;
>>> +
>>
>> /*
>> * Normal timing and ECC layout size setup is already done in
>> * the rk_nfc_select_chip() function.
>> */
>>
>> How about the ECC layout size setup for a boot block?
>>
>>> +	if (!buf)
>>> +	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
>>> +> +	for (i = 0; i < ecc->steps; i++) {
>>> +	/* Copy data to nfc buffer. */
>>> +	if (buf)
>>> +	memcpy(rk_nfc_data_ptr(chip, i),
>>> +	       rk_nfc_buf_to_data_ptr(chip, buf, i),
>>> +	       ecc->size);
>>
>>> +	/*
>>> +	* The first four bytes of OOB are reserved for the
>>> +	* boot ROM. In some debugging cases, such as with a
>>> +	* read, erase and write back test these 4 bytes stored
>>> +	* in OOB also need to be written back.
>>> +	*/
>>
>>
>> /*
>> * The first four bytes of OOB are reserved for the
>> * boot ROM. In some debugging cases, such as with a
>> * read, erase and write back test these 4 bytes stored
>> * in OOB also need to be written back.
>> *
>> * The function nand_block_bad detects bad blocks like:
>> *
>> * bad = chip->oob_poi[chip->badblockpos];
>> *
>> * chip->badblockpos == 0 for a large page NAND Flash,
>> * so chip->oob_poi[0] is the bad block mask (BBM).
>> *
>> * The OOB data layout on the NFC is:
>> *
>> *   PA0  PA1  PA2  PA3 | BBM OOB1 OOB2 OOB3 | ...
>> *
>> * or
>> *
>> *  0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
>> *
>> * The code here just swaps the first 4 bytes with the last
>> * 4 bytes without losing any data.
>> *
>> * The chip->oob_poi data layout:
>> *
>> *   BBM OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
>> *
>> * The rk_nfc_ooblayout_free() function already has reserved
>> * these 4 bytes with:
>> *
>> * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> */
>>
>>
>>> +	if (!i)
>>> +	memcpy(rk_nfc_oob_ptr(chip, i),
>>> +	       rk_nfc_buf_to_oob_ptr(chip, ecc->steps - 1),
>>> +	       NFC_SYS_DATA_SIZE);
>>> +	else
>>> +	memcpy(rk_nfc_oob_ptr(chip, i),
>>> +	       rk_nfc_buf_to_oob_ptr(chip, i - 1),
>>> +	       NFC_SYS_DATA_SIZE);
>>> +	/* Copy ECC data to the NFC buffer. */
>>> +	memcpy(rk_nfc_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
>>> +	       rk_nfc_buf_to_oob_ecc_ptr(chip, i),
>>> +	       ecc->bytes);
>>> +	}
>>> +
>>> +	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>>> +	rk_nfc_write_buf(nfc, buf, mtd->writesize + mtd->oobsize);
>>> +	ret = nand_prog_page_end_op(chip);
>>> +
>
>>> +	/*
>>> +	* Deselect the currently selected target after the ops is done
>>> +	* to reduce the power consumption.
>>> +	*/
>>> +	rk_nfc_select_chip(chip, -1);
>>
>> Does the MTD framework always select again?
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int rk_nfc_write_oob(struct nand_chip *chip, int page)
>>> +{
>>> +	return rk_nfc_write_page_raw(chip, NULL, 1, page);
>>> +}
>>> +
>>> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const 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;
>>> +	int ret = 0, i, boot_rom_mode = 0;
>>> +	dma_addr_t dma_data, dma_oob;
>>> +	u32 reg;
>>> +	u8 *oob;
>>> +
>>> +	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>>> +
>>> +	memcpy(nfc->page_buf, buf, mtd->writesize);
>>> +
>>> +	/*
>>> +	* The first blocks (4, 8 or 16 depending on the device) are used
>>> +	* by the boot ROM and the first 32 bits of OOB need to link to
>>> +	* the next page address in the same block. We can't directly copy
>>> +	* OOB data from the MTD framework, because this page address
>>> +	* conflicts for example with the bad block marker (BBM),
>>> +	* so we shift all OOB data including the BBM with 4 byte positions.
>>> +	* As a consequence the OOB size available to the MTD framework is
>>> +	* also reduced with 4 bytes.
>>> +	*
>>
>>> +	*    PA0 PA1 PA2 PA3 | BBM OOB1 OOB2 OOB3 | ...
>>
>>
>> *    PA0  PA1  PA2  PA3 | BBM OOB1 OOB2 OOB3 | ...
>>
>> keep layouts aligned
>>
>>> +	*
>>> +	* If a NAND is not a boot medium or the page is not a boot block,
>>> +	* the first 4 bytes are left untouched by writing 0xFF to them.
>>> +	*
>>> +	*   0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
>>> +	*
>>> +	* 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);
>>> +	}
>>> +
>>> +	for (i = 0; i < ecc->steps; i++) {
>>> +	if (!i) {
>>> +	reg = 0xFFFFFFFF;
>>> +	} else {
>>> +	oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
>>> +	reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
>>> +	      oob[3] << 24;
>>> +	}
>>> +	if (!i && boot_rom_mode)
>>> +	reg = (page & (pages_per_blk - 1)) * 4;
>>> +
>>> +	if (nfc->cfg->type == NFC_V9)
>>> +	nfc->oob_buf[i] = reg;
>>> +	else
>>> +	nfc->oob_buf[i * (oob_step / 4)] = reg;
>>> +	}
>>> +
>>> +	dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
>>> +	  mtd->writesize, DMA_TO_DEVICE);
>>> +	dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
>>> +	ecc->steps * oob_step,
>>> +	DMA_TO_DEVICE);
>>> +
>>> +	reinit_completion(&nfc->done);
>>> +	writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
>>> +
>>> +	rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
>>> +	  dma_oob);
>>> +	ret = wait_for_completion_timeout(&nfc->done,
>>> +	  msecs_to_jiffies(100));
>>> +	if (!ret)
>>> +	dev_warn(nfc->dev, "write: 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);
>>> +
>>> +	dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
>>> +	DMA_TO_DEVICE);
>>> +	dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
>>> +	DMA_TO_DEVICE);
>>> +
>>
>>> +	if (boot_rom_mode && rknand->boot_ecc != ecc->strength)
>>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>>> +
>>
>>> +	if (ret) {
>>> +	ret = -EIO;
>>> +	dev_err(nfc->dev,
>>> +	"write: wait transfer done timeout.\n");
>>> +	}
>>> +
>>
>>> +	if (ret)
>>> +	return ret;
>>
>> remove, always deselect
>>
>> if (!ret) {
>>
>>> +
>>> +	ret = nand_prog_page_end_op(chip);
>>
>> }
>>
>
>>> +
>>> +	/*
>>> +	* Deselect the currently selected target after the ops is done
>>> +	* to reduce the power consumption.
>>> +	*/
>>> +	rk_nfc_select_chip(chip, -1);
>>
>> Does the MTD framework always select again?
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int rk_nfc_read_page_raw(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 nand_ecc_ctrl *ecc = &chip->ecc;
>>> +	int i;
>>> +
>>
>> /*
>> * Normal timing and ECC layout size setup is already done in
>> * the rk_nfc_select_chip() function.
>> */
>>
>> How about the ECC layout size setup for a boot block?
>>
>>> +	nand_read_page_op(chip, page, 0, NULL, 0);
>>> +	rk_nfc_read_buf(nfc, nfc->buffer, mtd->writesize + mtd->oobsize);
>>> +
>
>>> +	/*
>>> +	* Deselect the currently selected target after the ops is done
>>> +	* to reduce the power consumption.
>>> +	*/
>>> +	rk_nfc_select_chip(chip, -1);
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>> +
>>> +	for (i = 0; i < ecc->steps; i++) {
>>> +	/*
>>> +	* The first four bytes of OOB are reserved for the
>>> +	* boot ROM. In some debugging cases, such as with a read,
>>> +	* erase and write back test, these 4 bytes also must be
>>> +	* saved somewhere, otherwise this information will be
>>> +	* lost during a write back.
>>> +	*/
>>> +	if (!i)
>>> +	memcpy(rk_nfc_buf_to_oob_ptr(chip, ecc->steps - 1),
>>> +	       rk_nfc_oob_ptr(chip, i),
>>> +	       NFC_SYS_DATA_SIZE);
>>> +	else
>>> +	memcpy(rk_nfc_buf_to_oob_ptr(chip, i - 1),
>>> +	       rk_nfc_oob_ptr(chip, i),
>>> +	       NFC_SYS_DATA_SIZE);
>>> +	/* Copy ECC data from the NFC buffer. */
>>> +	memcpy(rk_nfc_buf_to_oob_ecc_ptr(chip, i),
>>> +	       rk_nfc_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
>>> +	       ecc->bytes);
>>> +	/* Copy data from the NFC buffer. */
>>> +	if (buf)
>>> +	memcpy(rk_nfc_buf_to_data_ptr(chip, buf, i),
>>> +	       rk_nfc_data_ptr(chip, i),
>>> +	       ecc->size);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int rk_nfc_read_oob(struct nand_chip *chip, int page)
>>> +{
>>> +	return rk_nfc_read_page_raw(chip, NULL, 1, page);
>>> +}
>>> +
>>> +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);
>>
>> }
>>
>
>>> +
>>> +	/*
>>> +	* Deselect the currently selected target after the ops is done
>>> +	* to reduce the power consumption.
>>> +	*/
>>> +	rk_nfc_select_chip(chip, -1);
>
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>> +
>>
>>> +	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/