Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v13 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308,
 RK2928 and others
From:   Johan Jonker <jbx6244@gmail.com>
To:     Yifeng Zhao <yifeng.zhao@rock-chips.com>,
        miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
        robh+dt@kernel.org
Cc:     devicetree@vger.kernel.org, heiko@sntech.de,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org
References: <20201028095326.15562-1-yifeng.zhao@rock-chips.com>
 <20201028095326.15562-3-yifeng.zhao@rock-chips.com>
 <a8a7875b-f08b-62c6-a630-245687e0df3b@gmail.com>
Message-ID: <0dabd80e-b281-be65-b8e2-da00f46964fb@gmail.com>
Date:   Sat, 31 Oct 2020 14:45:37 +0100
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <a8a7875b-f08b-62c6-a630-245687e0df3b@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.
> 
>> +}
>> +