Hi Johan,

Johan Jonker [off-list ref] 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:

[..]

quoted

+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);
+  

quoted

+	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?

I don't know what link address is, but yes if it's an area used by the
controller for whatever reason it should be left alone, neither "free"
nor "ecc".

See further questions about this below.

quoted

+
+	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);
+  

quoted

+	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 ?

These functions do not generate anything and it's just a helper to
retrieve the ECC or free bytes, meaning that if you have 4 ECC bytes
per step, all concatenated, you will end with with an unique  ECC
section of 8 * 4 ECC bytes, this is perfectly fine.

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?

Yes please.

/*
 * 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,

fine

but are we still using it with HW ECC or only for raw?

both, you need to ensure reading a raw pages gives you a regular
data+ecc organization instead of the NFC's layout.

quoted

+
+	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,
+};  

[..]

quoted

+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?

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?

#################################################
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?

quoted

+
+		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;

quoted

+
+			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 '+'

quoted

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

[..]

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel