Message-ID: <2020061517300662418531@rock-chips.com>
Date:   Mon, 15 Jun 2020 17:34:14 +0800
From:   赵仪峰 <yifeng.zhao@...k-chips.com>
To:     "Johan Jonker" <jbx6244@...il.com>,
        "Miquel Raynal" <miquel.raynal@...tlin.com>,
        richard <richard@....at>, vigneshr <vigneshr@...com>,
        robh+dt <robh+dt@...nel.org>
Cc:     devicetree <devicetree@...r.kernel.org>,
        HeikoStübner <heiko@...ech.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-rockchip <linux-rockchip@...ts.infradead.org>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: Re: [PATCH v6 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others

Hi Johan,

Johan Jonker <jbx6244@...il.com> wrote on Sat, 13 Jun 2020 15:31:52
+0200:
>Hi Yifeng, Miquel,
>
>Some more comments about swap();
>
>On 6/9/20 9:40 AM, Yifeng Zhao wrote:
>
>[..]
>
>> +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>> +	struct mtd_oob_region *oob_region)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>
>> +	if (section >= chip->ecc.steps)
>> +	return -ERANGE;
>
>Given:
>
>NFC_SYS_DATA_SIZE = 4
>chip->ecc.steps = 8
>section [0..7]
>
>Total free OOB size advertised to the MTD framework is:
>
>ecc.steps * NFC_SYS_DATA_SIZE - 1 BBM
>8 * 4 - 1 = 31 bytes
>
>With link address in OOB byte [0..3] this become:
>31 - 4 = 27 bytes
>
>Does that give data lost?
>Should we limit the number of free OOB bytes by 4 more to be save?
>Is my calculation correct?
>See further questions about this below.
>
>> +
>> +	if (!section) {
>> +	/* The first byte is bad block mask flag. */
>> +	oob_region->length = NFC_SYS_DATA_SIZE - 1;
>> +	oob_region->offset = 1;
>> +	} else {
>> +	oob_region->length = NFC_SYS_DATA_SIZE;
>> +	oob_region->offset = section * NFC_SYS_DATA_SIZE;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +	struct mtd_oob_region *oob_region)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>
>> +	if (section)
>> +	return -ERANGE;
>
>With the formule above a section > 0 does not alow ECC.
>
>Just a question about the MTD inner working for Miquel:
>
>With ooblayout_free using 8 steps and this just 1 does it still generate
>the correct ECC? Does it calculate ECC over 1024B or over 8*1024B ?
>
>Should we move the text that explains the layout closer to these
>functions and add a little more text to explain why we choose this
>particular layout?
>
>/*
> * NFC Page Data Layout:
> *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> *	......
> * NAND Page Data Layout:
> *	1024 * n Data + m Bytes oob
> * Original Bad Block Mask Location:
> *	First byte of oob(spare).
> * nand_chip->oob_poi data layout:
> *	4Bytes sys data + .... + 4Bytes sys data + ecc data.
> */
>
>We expect now ECC data after n steps * 4 OOB bytes,
>but are we still using it with HW ECC or only for raw?
>
>> +
>> +	oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
>> +	oob_region->length = mtd->oobsize - oob_region->offset;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
>> +	.free = rk_nfc_ooblayout_free,
>> +	.ecc = rk_nfc_ooblayout_ecc,
>> +};
>
>[..]
>
>> +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +	     const u8 *buf, int page, int raw)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(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);
>> +
>> +	if (!raw) {
>> +	memcpy(nfc->page_buf, buf, mtd->writesize);
>> +	memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
>> +
>> +	/*
>> +	* The first 8(some devices are 4 or 16) blocks in use by
>> +	* the boot ROM and the first 32 bits of oob need to link
>> +	* to the next page address in the same block.
>> +	* Config the ECC algorithm supported by the boot ROM.
>> +	*/
>> +	if (page < pages_per_blk * rk_nand->boot_blks &&
>> +	    chip->options & NAND_IS_BOOT_MEDIUM) {
>> +	boot_rom_mode = 1;
>> +	if (rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc,
>> +	    rk_nand->boot_ecc);
>> +	}
>> +
>> +	/*
>> +	* Swap the first oob with the seventh oob and bad block
>> +	* mask is saved at the seventh oob.
>> +	*/
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>
>Add more info on why this is swapped.
>
>LA[0..3] is a link address that the BBM shouldn't over write.
>For Yifeng: Is there an other reason?

No other reason，this swap ops only for the link address.

>Before swap:
>
>BBM  OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 OOB7, OOB8 ....
>
>After swap:
>
>OOB7 OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 BBM , OOB8 ....
>
>If (!i && boot_rom_mode):
>
>LA0  LA1  LA2  LA3 , OOB4 OOB5 OOB6 BBM , OOB8 ....
>
>Read back after swap again:
>
>BBM  LA1  LA2  LA3 , OOB4 OOB5 OOB6 LA0 , OOB8 ....
>
>Question:
>Are data OOB7 OOB1 OOB2 OOB3 lost now?
>Is this correct?

Yes, the data OOB7 OOB1 OOB2 OOB3 will lost in the blocks which used for the boot ROM.

>#################################################
>Proposal:
>Should we reduce the free OOB size by 4
>and shift everything 4 bytes to recover all bytes?
>Replace the first 4 bytes with 0XFF or LA[0..3].
>
>Normal:
>0xFF 0XFF 0XFF 0xFF, BBM  OOB1 OOB2 OOB3, OOB4
>
>If (!i && boot_rom_mode):
>LA0  LA1  LA2  LA3 , BBM  OOB1 OOB2 OOB3, OOB4
>
>Question for Miquel and Yifeng:
>Does this work? Could you test?

I want to modify the drivers in next version:
The data swap ops only done for the blocks which used for the boot ROM，In this way,
the specially processed code will not affect the rest blocks.
For Miquel and Yifeng:
Is this OK？

>> +
>> +	for (i = 0; i < ecc->steps; i++) {
>
>Just a proposel:
>
>	if (!i && boot_rom_mode)
>	reg = (page & (pages_per_blk - 1)) * 4;
>	else 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 (nfc->cfg->type == NFC_V6 ||
>> +	    nfc->cfg->type == NFC_V8)
>> +	nfc->oob_buf[i * oob_step / 4] = reg;
>> +	else
>> +	nfc->oob_buf[i] = 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 && rk_nand->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");
>> +	}
>> +	} else {
>> +	rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize);
>
>Remove a '+'
>
>> +	}
>> +
>> +	if (ret)
>> +	return ret;
>> +
>> +	ret = nand_prog_page_end_op(chip);
>> +
>> +	/* Deselect the currently selected target. */
>> +	rk_nfc_select_chip(chip, -1);
>> +
>> +	return ret;
>> +}
>> +
>> +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);
>> +	u32 i;
>> +
>> +	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +	for (i = 0; i < chip->ecc.steps; i++) {
>> +	if (buf)
>> +	memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
>> +	       chip->ecc.size);
>> +
>> +	memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i),
>> +	       NFC_SYS_DATA_SIZE);
>> +	}
>> +
>> +	return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
>> +}
>> +
>> +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
>> +{
>> +	return rk_nfc_write_page_raw(chip, NULL, 1, page);
>> +}
>> +
>> +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +	    u32 data_offs, u32 readlen,
>> +	    u8 *buf, int page, int raw)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(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 bitflips = 0, bch_st;
>> +	u8 *oob;
>> +	u32 tmp;
>> +
>> +	nand_read_page_op(chip, page, 0, NULL, 0);
>> +	if (!raw) {
>> +	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 8(some devices are 4 or 16) blocks in use by
>> +	* the bootrom.
>> +	* Config the ECC algorithm supported by the boot ROM.
>> +	*/
>> +	if (page < pages_per_blk * rk_nand->boot_blks &&
>> +	    chip->options & NAND_IS_BOOT_MEDIUM) {
>> +	boot_rom_mode = 1;
>> +	if (rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc,
>> +	    rk_nand->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);
>> +	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;
>> +	dev_err(nfc->dev,
>> +	"read: wait transfer done timeout.\n");
>> +	goto out;
>> +	}
>> +
>> +	for (i = 0; i < ecc->steps; i++) {
>> +	oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
>> +	if (nfc->cfg->type == NFC_V6 ||
>> +	    nfc->cfg->type == NFC_V8)
>> +	tmp = nfc->oob_buf[i * oob_step / 4];
>> +	else
>> +	tmp = nfc->oob_buf[i];
>> +	*oob++ = (u8)tmp;
>> +	*oob++ = (u8)(tmp >> 8);
>> +	*oob++ = (u8)(tmp >> 16);
>> +	*oob++ = (u8)(tmp >> 24);
>> +	}
>> +
>> +	/*
>> +	* Swap the first oob with the seventh oob and bad block
>> +	* mask is saved at the seventh oob.
>> +	*/
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +
>> +	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 = -1;
>> +	} else {
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>> +	mtd->ecc_stats.corrected += ret;
>> +	bitflips = max_t(u32, bitflips, ret);
>> +
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>> +	mtd->ecc_stats.corrected += ret;
>> +	bitflips = max_t(u32, bitflips, ret);
>> +	}
>> +	}
>> +out:
>> +	memcpy(buf, nfc->page_buf, mtd->writesize);
>> +
>> +	if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>> +
>> +	if (bitflips < 0)
>> +	dev_err(nfc->dev, "read page: %x ecc error!\n", page);
>> +	} else {
>> +	rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
>> +	}
>> +	/* Deselect the currently selected target. */
>> +	rk_nfc_select_chip(chip, -1);
>> +
>> +	return bitflips;
>> +}
>> +
>> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>> +	   int oob_on, int page)
>> +{
>> +	return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
>> +}
>> +
>> +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
>> +	  int pg)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +
>> +	return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
>> +}
>> +
>> +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);
>> +	int i, ret;
>> +
>> +	ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
>> +	       page, 1);
>> +	if (ret < 0)
>> +	return ret;
>> +
>> +	for (i = 0; i < chip->ecc.steps; i++) {
>> +	memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i),
>> +	       NFC_SYS_DATA_SIZE);
>> +
>> +	if (buf)
>> +	memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
>> +	       chip->ecc.size);
>> +	}
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +
>> +	return ret;
>> +}
>
>[..]
>
>
>

Powered by blists - more mailing lists