[PATCH 0/2] mtd: nand: sunxi: cleanup

[PATCH 0/2] mtd: nand: sunxi: cleanup

[Boris Brezillon]

Hello,

Those two patches aim at cleaning up the sunxi_nand driver by first adding
some consistency in the macro definitions, and then factorizing the code
duplicated code found in hw_ecc and hw_syndrome_ecc implementations.

Best Regards,

Boris

Boris Brezillon (2):
  mtd: nand: sunxi: rework macros
  mtd: sunxi: nand: refactor ->read_page()/->write_page() code

 drivers/mtd/nand/sunxi_nand.c | 487 ++++++++++++++++++++++--------------------
 1 file changed, 256 insertions(+), 231 deletions(-)

-- 
1.9.1

[PATCH 1/2] mtd: nand: sunxi: rework macros

[Boris Brezillon]

Suffix mask macros with _MSK and add new helper macros to avoid manually
shifting values.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand.c | 123 ++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index ef1fe61..dc44435 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -57,11 +57,8 @@
 #define NFC_REG_ECC_CTL		0x0034
 #define NFC_REG_ECC_ST		0x0038
 #define NFC_REG_DEBUG		0x003C
-#define NFC_REG_ECC_CNT0	0x0040
-#define NFC_REG_ECC_CNT1	0x0044
-#define NFC_REG_ECC_CNT2	0x0048
-#define NFC_REG_ECC_CNT3	0x004c
-#define NFC_REG_USER_DATA_BASE	0x0050
+#define NFC_REG_ECC_ERR_CNT(x)	((0x0040 + (x)) & ~0x3)
+#define NFC_REG_USER_DATA(x)	(0x0050 + ((x) * 4))
 #define NFC_REG_SPARE_AREA	0x00A0
 #define NFC_RAM0_BASE		0x0400
 #define NFC_RAM1_BASE		0x0800
@@ -69,12 +66,16 @@
 /* define bit use in NFC_CTL */
 #define NFC_EN			BIT(0)
 #define NFC_RESET		BIT(1)
-#define NFC_BUS_WIDYH		BIT(2)
-#define NFC_RB_SEL		BIT(3)
-#define NFC_CE_SEL		GENMASK(26, 24)
+#define NFC_BUS_WIDTH_MSK	BIT(2)
+#define NFC_BUS_WIDTH_8		(0 << 2)
+#define NFC_BUS_WIDTH_16	(1 << 2)
+#define NFC_RB_SEL_MSK		BIT(3)
+#define NFC_RB_SEL(x)		((x) << 3)
+#define NFC_CE_SEL_MSK		GENMASK(26, 24)
+#define NFC_CE_SEL(x)		((x) << 24)
 #define NFC_CE_CTL		BIT(6)
-#define NFC_CE_CTL1		BIT(7)
-#define NFC_PAGE_SIZE		GENMASK(11, 8)
+#define NFC_PAGE_SHIFT_MSK	GENMASK(11, 8)
+#define NFC_PAGE_SHIFT(x)	(((x) < 10 ? 0 : (x) - 10) << 8)
 #define NFC_SAM			BIT(12)
 #define NFC_RAM_METHOD		BIT(14)
 #define NFC_DEBUG_CTL		BIT(31)
@@ -86,10 +87,7 @@
 #define NFC_CMD_FIFO_STATUS	BIT(3)
 #define NFC_STA			BIT(4)
 #define NFC_NATCH_INT_FLAG	BIT(5)
-#define NFC_RB_STATE0		BIT(8)
-#define NFC_RB_STATE1		BIT(9)
-#define NFC_RB_STATE2		BIT(10)
-#define NFC_RB_STATE3		BIT(11)
+#define NFC_RB_STATE(x)		BIT(x + 8)
 
 /* define bit use in NFC_INT */
 #define NFC_B2R_INT_ENABLE	BIT(0)
@@ -109,9 +107,11 @@
 	(((tCAD) & 0x7) << 8))
 
 /* define bit use in NFC_CMD */
-#define NFC_CMD_LOW_BYTE	GENMASK(7, 0)
-#define NFC_CMD_HIGH_BYTE	GENMASK(15, 8)
-#define NFC_ADR_NUM		GENMASK(18, 16)
+#define NFC_CMD_LOW_BYTE_MSK	GENMASK(7, 0)
+#define NFC_CMD_HIGH_BYTE_MSK	GENMASK(15, 8)
+#define NFC_CMD(x)		(x)
+#define NFC_ADR_NUM_MSK		GENMASK(18, 16)
+#define NFC_ADR_NUM(x)		(((x) - 1) << 16)
 #define NFC_SEND_ADR		BIT(19)
 #define NFC_ACCESS_DIR		BIT(20)
 #define NFC_DATA_TRANS		BIT(21)
@@ -123,29 +123,38 @@
 #define NFC_ROW_AUTO_INC	BIT(27)
 #define NFC_SEND_CMD3		BIT(28)
 #define NFC_SEND_CMD4		BIT(29)
-#define NFC_CMD_TYPE		GENMASK(31, 30)
+#define NFC_CMD_TYPE_MSK	GENMASK(31, 30)
+#define NFC_NORMAL_OP		(0 << 30)
+#define NFC_ECC_OP		(1 << 30)
+#define NFC_PAGE_OP		(2 << 30)
 
 /* define bit use in NFC_RCMD_SET */
-#define NFC_READ_CMD		GENMASK(7, 0)
-#define NFC_RANDOM_READ_CMD0	GENMASK(15, 8)
-#define NFC_RANDOM_READ_CMD1	GENMASK(23, 16)
+#define NFC_READ_CMD_MSK	GENMASK(7, 0)
+#define NFC_RND_READ_CMD0_MSK	GENMASK(15, 8)
+#define NFC_RND_READ_CMD1_MSK	GENMASK(23, 16)
 
 /* define bit use in NFC_WCMD_SET */
-#define NFC_PROGRAM_CMD		GENMASK(7, 0)
-#define NFC_RANDOM_WRITE_CMD	GENMASK(15, 8)
-#define NFC_READ_CMD0		GENMASK(23, 16)
-#define NFC_READ_CMD1		GENMASK(31, 24)
+#define NFC_PROGRAM_CMD_MSK	GENMASK(7, 0)
+#define NFC_RND_WRITE_CMD_MSK	GENMASK(15, 8)
+#define NFC_READ_CMD0_MSK	GENMASK(23, 16)
+#define NFC_READ_CMD1_MSK	GENMASK(31, 24)
 
 /* define bit use in NFC_ECC_CTL */
 #define NFC_ECC_EN		BIT(0)
 #define NFC_ECC_PIPELINE	BIT(3)
 #define NFC_ECC_EXCEPTION	BIT(4)
-#define NFC_ECC_BLOCK_SIZE	BIT(5)
+#define NFC_ECC_BLOCK_SIZE_MSK	BIT(5)
 #define NFC_RANDOM_EN		BIT(9)
 #define NFC_RANDOM_DIRECTION	BIT(10)
-#define NFC_ECC_MODE_SHIFT	12
-#define NFC_ECC_MODE		GENMASK(15, 12)
-#define NFC_RANDOM_SEED		GENMASK(30, 16)
+#define NFC_ECC_MODE_MSK	GENMASK(15, 12)
+#define NFC_ECC_MODE(x)		((x) << 12)
+#define NFC_RANDOM_SEED_MSK	GENMASK(30, 16)
+#define NFC_RANDOM_SEED(x)	((x) << 16)
+
+/* define bit use in NFC_ECC_ST */
+#define NFC_ECC_ERR(x)		BIT(x)
+#define NFC_ECC_PAT_FOUND(x)	BIT(x + 16)
+#define NFC_ECC_ERR_CNT(b, x)	(((x) >> ((b) * 8)) & 0xff)
 
 /* NFC_USER_DATA helper macros */
 #define NFC_BUF_TO_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
@@ -360,13 +369,13 @@ static int sunxi_nfc_dev_ready(struct mtd_info *mtd)
 	switch (rb->type) {
 	case RB_NATIVE:
 		ret = !!(readl(nfc->regs + NFC_REG_ST) &
-			 (NFC_RB_STATE0 << rb->info.nativeid));
+			 NFC_RB_STATE(rb->info.nativeid));
 		if (ret)
 			break;
 
 		sunxi_nfc_wait_int(nfc, NFC_RB_B2R, timeo);
 		ret = !!(readl(nfc->regs + NFC_REG_ST) &
-			 (NFC_RB_STATE0 << rb->info.nativeid));
+			 NFC_RB_STATE(rb->info.nativeid));
 		break;
 	case RB_GPIO:
 		ret = gpio_get_value(rb->info.gpio);
@@ -396,19 +405,19 @@ static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip)
 		return;
 
 	ctl = readl(nfc->regs + NFC_REG_CTL) &
-	      ~(NFC_CE_SEL | NFC_RB_SEL | NFC_EN);
+	      ~(NFC_PAGE_SHIFT_MSK | NFC_CE_SEL_MSK | NFC_RB_SEL_MSK | NFC_EN);
 
 	if (chip >= 0) {
 		sel = &sunxi_nand->sels[chip];
 
-		ctl |= (sel->cs << 24) | NFC_EN |
-		       (((nand->page_shift - 10) & 0xf) << 8);
+		ctl |= NFC_CE_SEL(sel->cs) | NFC_EN |
+		       NFC_PAGE_SHIFT(nand->page_shift - 10);
 		if (sel->rb.type == RB_NONE) {
 			nand->dev_ready = NULL;
 		} else {
 			nand->dev_ready = sunxi_nfc_dev_ready;
 			if (sel->rb.type == RB_NATIVE)
-				ctl |= (sel->rb.info.nativeid << 3);
+				ctl |= NFC_RB_SEL(sel->rb.info.nativeid);
 		}
 
 		writel(mtd->writesize, nfc->regs + NFC_REG_SPARE_AREA);
@@ -550,9 +559,8 @@ static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
 	int cnt;
 
 	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE);
-	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT) |
-	       NFC_ECC_EXCEPTION;
+	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
+	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
 
 	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
 
@@ -570,7 +578,7 @@ static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
 		if (ret)
 			return ret;
 
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP;
 		writel(tmp, nfc->regs + NFC_REG_CMD);
 
 		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
@@ -580,11 +588,11 @@ static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
 		memcpy_fromio(buf + (i * ecc->size),
 			      nfc->regs + NFC_RAM0_BASE, ecc->size);
 
-		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
+		if (readl(nfc->regs + NFC_REG_ECC_ST) & NFC_ECC_ERR(0)) {
 			mtd->ecc_stats.failed++;
 		} else {
-			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
-			mtd->ecc_stats.corrected += tmp;
+			tmp = readl(nfc->regs + NFC_REG_ECC_ERR_CNT(0));
+			mtd->ecc_stats.corrected += NFC_ECC_ERR_CNT(0, tmp);
 			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
 		}
 
@@ -635,9 +643,8 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
 	int cnt;
 
 	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE);
-	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT) |
-	       NFC_ECC_EXCEPTION;
+	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
+	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
 
 	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
 
@@ -652,7 +659,7 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
 		/* Fill OOB data in */
 		writel(NFC_BUF_TO_USER_DATA(chip->oob_poi +
 					    layout->oobfree[i].offset),
-		       nfc->regs + NFC_REG_USER_DATA_BASE);
+		       nfc->regs + NFC_REG_USER_DATA(0));
 
 		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
 
@@ -661,7 +668,7 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
 			return ret;
 
 		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
-		      (1 << 30);
+		      NFC_ECC_OP;
 		writel(tmp, nfc->regs + NFC_REG_CMD);
 		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
 		if (ret)
@@ -704,16 +711,15 @@ static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
 	int i;
 
 	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE);
-	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT) |
-	       NFC_ECC_EXCEPTION;
+	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
+	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
 
 	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
 
 	for (i = 0; i < ecc->steps; i++) {
 		chip->read_buf(mtd, NULL, ecc->size);
 
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP;
 		writel(tmp, nfc->regs + NFC_REG_CMD);
 
 		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
@@ -724,11 +730,11 @@ static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
 		buf += ecc->size;
 		offset += ecc->size;
 
-		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
+		if (readl(nfc->regs + NFC_REG_ECC_ST) & NFC_ECC_ERR(0)) {
 			mtd->ecc_stats.failed++;
 		} else {
-			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
-			mtd->ecc_stats.corrected += tmp;
+			tmp = readl(nfc->regs + NFC_REG_ECC_ERR_CNT(0));
+			mtd->ecc_stats.corrected += NFC_ECC_ERR_CNT(0, tmp);
 			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
 		}
 
@@ -771,9 +777,8 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
 	int i;
 
 	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE);
-	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT) |
-	       NFC_ECC_EXCEPTION;
+	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
+	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
 
 	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
 
@@ -783,10 +788,10 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
 
 		/* Fill OOB data in */
 		writel(NFC_BUF_TO_USER_DATA(oob),
-		       nfc->regs + NFC_REG_USER_DATA_BASE);
+		       nfc->regs + NFC_REG_USER_DATA(0));
 
 		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
-		      (1 << 30);
+		      NFC_ECC_OP;
 		writel(tmp, nfc->regs + NFC_REG_CMD);
 
 		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
-- 
1.9.1

[PATCH 2/2] mtd: sunxi: nand: refactor ->read_page()/->write_page() code

[Boris Brezillon]

Most of the logic to read/write pages with the HW ECC engine enabled
is common to the HW_ECC and NAND_ECC_HW_SYNDROME scheme.
Refactor the code to avoid code duplication.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand.c | 404 ++++++++++++++++++++++--------------------
 1 file changed, 212 insertions(+), 192 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index dc44435..640b96c 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -159,6 +159,13 @@
 /* NFC_USER_DATA helper macros */
 #define NFC_BUF_TO_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
 					((buf)[2] << 16) | ((buf)[3] << 24))
+#define NFC_USER_DATA_TO_BUF(buf, val)	\
+	{				\
+		(buf)[0] = val;		\
+		(buf)[1] = val >> 8;	\
+		(buf)[2] = val >> 16;	\
+		(buf)[3] = val >> 24;	\
+	}
 
 #define NFC_DEFAULT_TIMEOUT_MS	1000
 
@@ -543,153 +550,228 @@ static void sunxi_nfc_cmd_ctrl(struct mtd_info *mtd, int dat,
 	sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
 }
 
-static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
-				      struct nand_chip *chip, uint8_t *buf,
-				      int oob_required, int page)
+static void sunxi_nfc_hw_ecc_enable(struct mtd_info *mtd)
 {
-	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
-	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	struct nand_ecclayout *layout = ecc->layout;
-	struct sunxi_nand_hw_ecc *data = ecc->priv;
-	unsigned int max_bitflips = 0;
-	int offset;
-	int ret;
-	u32 tmp;
-	int i;
-	int cnt;
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+	struct sunxi_nand_hw_ecc *data = nand->ecc.priv;
+	u32 ecc_ctl;
 
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
-	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
+	ecc_ctl = readl(nfc->regs + NFC_REG_ECC_CTL);
+	ecc_ctl &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE |
+		     NFC_ECC_BLOCK_SIZE_MSK);
+	ecc_ctl |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	writel(ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
+}
 
-	for (i = 0; i < ecc->steps; i++) {
-		if (i)
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, i * ecc->size, -1);
+static void sunxi_nfc_hw_ecc_disable(struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
 
-		offset = mtd->writesize + layout->eccpos[i * ecc->bytes] - 4;
+	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
+	       nfc->regs + NFC_REG_ECC_CTL);
+}
 
-		chip->read_buf(mtd, NULL, ecc->size);
+static int sunxi_nfc_hw_ecc_read_chunk(struct mtd_info *mtd,
+				       u8 *data, int data_off,
+				       u8 *oob, int oob_off,
+				       int *cur_off,
+				       unsigned int *max_bitflips)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+	struct nand_ecc_ctrl *ecc = &nand->ecc;
+	u32 status;
+	int ret;
 
-		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+	if (*cur_off != data_off)
+		nand->cmdfunc(mtd, NAND_CMD_RNDOUT, data_off, -1);
 
-		ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
-		if (ret)
-			return ret;
+	sunxi_nfc_read_buf(mtd, data, ecc->size);
 
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP;
-		writel(tmp, nfc->regs + NFC_REG_CMD);
+	if (data_off + ecc->bytes != oob_off)
+		nand->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_off, -1);
 
-		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
-		if (ret)
-			return ret;
+	ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
+	if (ret)
+		return ret;
 
-		memcpy_fromio(buf + (i * ecc->size),
-			      nfc->regs + NFC_RAM0_BASE, ecc->size);
+	writel(NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP,
+	       nfc->regs + NFC_REG_CMD);
 
-		if (readl(nfc->regs + NFC_REG_ECC_ST) & NFC_ECC_ERR(0)) {
-			mtd->ecc_stats.failed++;
-		} else {
-			tmp = readl(nfc->regs + NFC_REG_ECC_ERR_CNT(0));
-			mtd->ecc_stats.corrected += NFC_ECC_ERR_CNT(0, tmp);
-			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
-		}
+	ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+	if (ret)
+		return ret;
 
-		if (oob_required) {
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+	status = readl(nfc->regs + NFC_REG_ECC_ST);
+	ret = NFC_ECC_ERR_CNT(0, readl(nfc->regs + NFC_REG_ECC_ERR_CNT(0)));
 
-			ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
-			if (ret)
-				return ret;
+	memcpy_fromio(data, nfc->regs + NFC_RAM0_BASE, ecc->size);
 
-			offset -= mtd->writesize;
-			chip->read_buf(mtd, chip->oob_poi + offset,
-				      ecc->bytes + 4);
-		}
+	nand->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_off, -1);
+	sunxi_nfc_read_buf(mtd, oob, ecc->bytes + 4);
+
+	if (status & NFC_ECC_ERR(0)) {
+		ret = -EIO;
+	} else {
+		/*
+		 * The engine protects 4 bytes OOB data per chunk.
+		 * Retrieve the corrected OOB bytes.
+		 */
+		NFC_USER_DATA_TO_BUF(oob,
+				     readl(nfc->regs + NFC_REG_USER_DATA(0)));
 	}
 
-	if (oob_required) {
-		cnt = ecc->layout->oobfree[ecc->steps].length;
-		if (cnt > 0) {
-			offset = mtd->writesize +
-				 ecc->layout->oobfree[ecc->steps].offset;
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
-			offset -= mtd->writesize;
-			chip->read_buf(mtd, chip->oob_poi + offset, cnt);
-		}
+	if (ret < 0) {
+		mtd->ecc_stats.failed++;
+	} else {
+		mtd->ecc_stats.corrected += ret;
+		*max_bitflips = max_t(unsigned int, *max_bitflips, ret);
 	}
 
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~NFC_ECC_EN;
+	*cur_off = oob_off + ecc->bytes + 4;
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	return 0;
+}
 
-	return max_bitflips;
+static void sunxi_nfc_hw_ecc_read_extra_oob(struct mtd_info *mtd,
+					    u8 *oob, int *cur_off)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct nand_ecc_ctrl *ecc = &nand->ecc;
+	int offset = ((ecc->bytes + 4) * ecc->steps);
+	int len = mtd->oobsize - offset;
+
+	if (len <= 0)
+		return;
+
+	if (*cur_off != offset)
+		nand->cmdfunc(mtd, NAND_CMD_RNDOUT,
+			      offset + mtd->writesize, -1);
+
+	sunxi_nfc_read_buf(mtd, oob + offset, len);
+
+	*cur_off = mtd->oobsize + mtd->writesize;
 }
 
-static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
-				       struct nand_chip *chip,
-				       const uint8_t *buf, int oob_required)
+static int sunxi_nfc_hw_ecc_write_chunk(struct mtd_info *mtd,
+					const u8 *data, int data_off,
+					const u8 *oob, int oob_off,
+					int *cur_off)
 {
-	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
-	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	struct nand_ecclayout *layout = ecc->layout;
-	struct sunxi_nand_hw_ecc *data = ecc->priv;
-	int offset;
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+	struct nand_ecc_ctrl *ecc = &nand->ecc;
 	int ret;
-	u32 tmp;
-	int i;
-	int cnt;
 
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
-	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
+	if (data_off != *cur_off)
+		nand->cmdfunc(mtd, NAND_CMD_RNDIN, data_off, -1);
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_write_buf(mtd, data, ecc->size);
 
-	for (i = 0; i < ecc->steps; i++) {
-		if (i)
-			chip->cmdfunc(mtd, NAND_CMD_RNDIN, i * ecc->size, -1);
+	/* Fill OOB data in */
+	writel(NFC_BUF_TO_USER_DATA(oob), nfc->regs + NFC_REG_USER_DATA(0));
 
-		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
+	if (data_off + ecc->bytes != oob_off)
+		nand->cmdfunc(mtd, NAND_CMD_RNDIN, oob_off, -1);
 
-		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
+	ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
+	if (ret)
+		return ret;
 
-		/* Fill OOB data in */
-		writel(NFC_BUF_TO_USER_DATA(chip->oob_poi +
-					    layout->oobfree[i].offset),
-		       nfc->regs + NFC_REG_USER_DATA(0));
+	writel(NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD |
+	       NFC_ACCESS_DIR | NFC_ECC_OP,
+	       nfc->regs + NFC_REG_CMD);
 
-		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
+	ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+	if (ret)
+		return ret;
 
-		ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
-		if (ret)
-			return ret;
+	*cur_off = oob_off + ecc->bytes + 4;
 
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
-		      NFC_ECC_OP;
-		writel(tmp, nfc->regs + NFC_REG_CMD);
-		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+	return 0;
+}
+
+static void sunxi_nfc_hw_ecc_write_extra_oob(struct mtd_info *mtd,
+					     u8 *oob, int *cur_off)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct nand_ecc_ctrl *ecc = &nand->ecc;
+	int offset = ((ecc->bytes + 4) * ecc->steps);
+	int len = mtd->oobsize - offset;
+
+	if (len <= 0)
+		return;
+
+	if (*cur_off != offset)
+		nand->cmdfunc(mtd, NAND_CMD_RNDIN,
+			      offset + mtd->writesize, -1);
+
+	sunxi_nfc_write_buf(mtd, oob + offset, len);
+
+	*cur_off = mtd->oobsize + mtd->writesize;
+}
+
+static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
+				      struct nand_chip *chip, uint8_t *buf,
+				      int oob_required, int page)
+{
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	unsigned int max_bitflips = 0;
+	int ret, i, cur_off = 0;
+
+	sunxi_nfc_hw_ecc_enable(mtd);
+
+	for (i = 0; i < ecc->steps; i++) {
+		int data_off = i * ecc->size;
+		int oob_off = i * (ecc->bytes + 4);
+		u8 *data = buf + data_off;
+		u8 *oob = chip->oob_poi + oob_off;
+
+		ret = sunxi_nfc_hw_ecc_read_chunk(mtd, data, data_off, oob,
+						  oob_off + mtd->writesize,
+						  &cur_off, &max_bitflips);
 		if (ret)
 			return ret;
 	}
 
-	if (oob_required) {
-		cnt = ecc->layout->oobfree[i].length;
-		if (cnt > 0) {
-			offset = mtd->writesize +
-				 ecc->layout->oobfree[i].offset;
-			chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
-			offset -= mtd->writesize;
-			chip->write_buf(mtd, chip->oob_poi + offset, cnt);
-		}
+	if (oob_required)
+		sunxi_nfc_hw_ecc_read_extra_oob(mtd, chip->oob_poi,
+						&cur_off);
+
+	sunxi_nfc_hw_ecc_disable(mtd);
+
+	return max_bitflips;
+}
+
+static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
+				       struct nand_chip *chip,
+				       const uint8_t *buf, int oob_required)
+{
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int ret, i, cur_off = 0;
+
+	sunxi_nfc_hw_ecc_enable(mtd);
+
+	for (i = 0; i < ecc->steps; i++) {
+		int data_off = i * ecc->size;
+		int oob_off = i * (ecc->bytes + 4);
+		const u8 *data = buf + data_off;
+		const u8 *oob = chip->oob_poi + oob_off;
+
+		ret = sunxi_nfc_hw_ecc_write_chunk(mtd, data, data_off, oob,
+						   oob_off + mtd->writesize,
+						   &cur_off);
+		if (ret)
+			return ret;
 	}
 
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~NFC_ECC_EN;
+	if (oob_required)
+		sunxi_nfc_hw_ecc_write_extra_oob(mtd, chip->oob_poi, &cur_off);
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_hw_ecc_disable(mtd);
 
 	return 0;
 }
@@ -699,64 +781,29 @@ static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
 					       uint8_t *buf, int oob_required,
 					       int page)
 {
-	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	struct sunxi_nand_hw_ecc *data = ecc->priv;
 	unsigned int max_bitflips = 0;
-	uint8_t *oob = chip->oob_poi;
-	int offset = 0;
-	int ret;
-	int cnt;
-	u32 tmp;
-	int i;
-
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
-	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
+	int ret, i, cur_off = 0;
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_hw_ecc_enable(mtd);
 
 	for (i = 0; i < ecc->steps; i++) {
-		chip->read_buf(mtd, NULL, ecc->size);
-
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP;
-		writel(tmp, nfc->regs + NFC_REG_CMD);
-
-		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		int data_off = i * (ecc->size + ecc->bytes + 4);
+		int oob_off = data_off + ecc->size;
+		u8 *data = buf + (i * ecc->size);
+		u8 *oob = chip->oob_poi + (i * (ecc->bytes + 4));
+
+		ret = sunxi_nfc_hw_ecc_read_chunk(mtd, data, data_off, oob,
+						  oob_off, &cur_off,
+						  &max_bitflips);
 		if (ret)
 			return ret;
-
-		memcpy_fromio(buf, nfc->regs + NFC_RAM0_BASE, ecc->size);
-		buf += ecc->size;
-		offset += ecc->size;
-
-		if (readl(nfc->regs + NFC_REG_ECC_ST) & NFC_ECC_ERR(0)) {
-			mtd->ecc_stats.failed++;
-		} else {
-			tmp = readl(nfc->regs + NFC_REG_ECC_ERR_CNT(0));
-			mtd->ecc_stats.corrected += NFC_ECC_ERR_CNT(0, tmp);
-			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
-		}
-
-		if (oob_required) {
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
-			chip->read_buf(mtd, oob, ecc->bytes + ecc->prepad);
-			oob += ecc->bytes + ecc->prepad;
-		}
-
-		offset += ecc->bytes + ecc->prepad;
 	}
 
-	if (oob_required) {
-		cnt = mtd->oobsize - (oob - chip->oob_poi);
-		if (cnt > 0) {
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
-			chip->read_buf(mtd, oob, cnt);
-		}
-	}
+	if (oob_required)
+		sunxi_nfc_hw_ecc_read_extra_oob(mtd, chip->oob_poi, &cur_off);
 
-	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
-	       nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_hw_ecc_disable(mtd);
 
 	return max_bitflips;
 }
@@ -766,54 +813,27 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
 						const uint8_t *buf,
 						int oob_required)
 {
-	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	struct sunxi_nand_hw_ecc *data = ecc->priv;
-	uint8_t *oob = chip->oob_poi;
-	int offset = 0;
-	int ret;
-	int cnt;
-	u32 tmp;
-	int i;
-
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE_MSK);
-	tmp |= NFC_ECC_EN | NFC_ECC_MODE(data->mode) | NFC_ECC_EXCEPTION;
+	int ret, i, cur_off = 0;
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_hw_ecc_enable(mtd);
 
 	for (i = 0; i < ecc->steps; i++) {
-		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
-		offset += ecc->size;
+		int data_off = i * (ecc->size + ecc->bytes + 4);
+		int oob_off = data_off + ecc->size;
+		const u8 *data = buf + (i * ecc->size);
+		const u8 *oob = chip->oob_poi + (i * (ecc->bytes + 4));
 
-		/* Fill OOB data in */
-		writel(NFC_BUF_TO_USER_DATA(oob),
-		       nfc->regs + NFC_REG_USER_DATA(0));
-
-		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
-		      NFC_ECC_OP;
-		writel(tmp, nfc->regs + NFC_REG_CMD);
-
-		ret = sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		ret = sunxi_nfc_hw_ecc_write_chunk(mtd, data, data_off,
+						   oob, oob_off, &cur_off);
 		if (ret)
 			return ret;
-
-		offset += ecc->bytes + ecc->prepad;
-		oob += ecc->bytes + ecc->prepad;
-	}
-
-	if (oob_required) {
-		cnt = mtd->oobsize - (oob - chip->oob_poi);
-		if (cnt > 0) {
-			chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
-			chip->write_buf(mtd, oob, cnt);
-		}
 	}
 
-	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
-	tmp &= ~NFC_ECC_EN;
+	if (oob_required)
+		sunxi_nfc_hw_ecc_write_extra_oob(mtd, chip->oob_poi, &cur_off);
 
-	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+	sunxi_nfc_hw_ecc_disable(mtd);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 0/2] mtd: nand: sunxi: cleanup

[Brian Norris]

On Wed, Sep 16, 2015 at 09:46:35AM +0200, Boris Brezillon wrote:
> Hello,
> 
> Those two patches aim at cleaning up the sunxi_nand driver by first adding
> some consistency in the macro definitions, and then factorizing the code
> duplicated code found in hw_ecc and hw_syndrome_ecc implementations.
> 
> Best Regards,
> 
> Boris

I believe this series no longer applies cleanly

[PATCH 0/2] mtd: nand: sunxi: cleanup

[Boris Brezillon]

Hi Brian,

On Tue, 29 Sep 2015 15:21:44 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Wed, Sep 16, 2015 at 09:46:35AM +0200, Boris Brezillon wrote:
> > Hello,
> > 
> > Those two patches aim at cleaning up the sunxi_nand driver by first adding
> > some consistency in the macro definitions, and then factorizing the code
> > duplicated code found in hw_ecc and hw_syndrome_ecc implementations.
> > 
> > Best Regards,
> > 
> > Boris
> 
> I believe this series no longer applies cleanly

Hm, actually it depends on fixes you have in your linux-mtd tree and
features in your l2-mtd tree.

I don't know how you usually deal with those problems (merging the
linux-mtd/master branch into the l2-mtd/master one, or merging
last linus' -rc into l2-mtd/master should work), but I'd really
like to have this in 4.4 :-).
I still have the patch using the nand_check_erased_ecc_chunk() function
which depends on those 2 patches, and I'd like to have it in 4.4 too.

I know I'm asking a lot, but as explained earlier, I still have a
bunch of patches on top of those already posted (mainly to support
the hardware randomizer), and I'd like the trivial ones to be merged
quickly.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/2] mtd: nand: sunxi: cleanup

[Brian Norris]

On Wed, Sep 30, 2015 at 08:36:34AM +0200, Boris Brezillon wrote:
> On Tue, 29 Sep 2015 15:21:44 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > On Wed, Sep 16, 2015 at 09:46:35AM +0200, Boris Brezillon wrote:
> > > Hello,
> > > 
> > > Those two patches aim at cleaning up the sunxi_nand driver by first adding
> > > some consistency in the macro definitions, and then factorizing the code
> > > duplicated code found in hw_ecc and hw_syndrome_ecc implementations.
> > > 
> > > Best Regards,
> > > 
> > > Boris
> > 
> > I believe this series no longer applies cleanly
> 
> Hm, actually it depends on fixes you have in your linux-mtd tree and
> features in your l2-mtd tree.
> 
> I don't know how you usually deal with those problems (merging the
> linux-mtd/master branch into the l2-mtd/master one, or merging
> last linus' -rc into l2-mtd/master should work), but I'd really
> like to have this in 4.4 :-).

linux-mtd.git is prepped for a pull request. I'll do that before the
week ends. And I'll either merge that back into l2-mtd.git at that time,
or just pull in v4.3-rc4.

> I still have the patch using the nand_check_erased_ecc_chunk() function
> which depends on those 2 patches, and I'd like to have it in 4.4 too.
> 
> I know I'm asking a lot, but as explained earlier, I still have a
> bunch of patches on top of those already posted (mainly to support
> the hardware randomizer), and I'd like the trivial ones to be merged
> quickly.

I'll see what I can apply after pulling together all the latest, but if
I'm still missing things then, feel free to resend.

BTW, I thought there were some things to address on the erased ECC stuff
still. I'll re-check that series too.

Brian

[PATCH 1/2] mtd: nand: sunxi: rework macros

[Brian Norris]

On Wed, Sep 16, 2015 at 09:46:36AM +0200, Boris Brezillon wrote:
> Suffix mask macros with _MSK and add new helper macros to avoid manually
> shifting values.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Pushed patch 1 to l2-mtd.git

[PATCH 2/2] mtd: sunxi: nand: refactor ->read_page()/->write_page() code

[Brian Norris]

On Wed, Sep 16, 2015 at 09:46:37AM +0200, Boris Brezillon wrote:
> Most of the logic to read/write pages with the HW ECC engine enabled
> is common to the HW_ECC and NAND_ECC_HW_SYNDROME scheme.
> Refactor the code to avoid code duplication.

Hmm, a benign commit description to describe a somewhat complicated
patch. This seems to do several different types of refactoring all at
once, and it makes it a bit hard to review. Can you perhaps refactor
this into 2 or more patches? e.g., I think the NFC_USER_DATA_TO_BUF()
stuff can be orthogonal from the introduction of
sunxi_nfc_hw_ecc_read_chunk() and sunxi_nfc_hw_ecc_read_extra_oob().

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/sunxi_nand.c | 404 ++++++++++++++++++++++--------------------
>  1 file changed, 212 insertions(+), 192 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> index dc44435..640b96c 100644
> --- a/drivers/mtd/nand/sunxi_nand.c
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -159,6 +159,13 @@
>  /* NFC_USER_DATA helper macros */
>  #define NFC_BUF_TO_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
>  					((buf)[2] << 16) | ((buf)[3] << 24))
> +#define NFC_USER_DATA_TO_BUF(buf, val)	\
> +	{				\
> +		(buf)[0] = val;		\
> +		(buf)[1] = val >> 8;	\
> +		(buf)[2] = val >> 16;	\
> +		(buf)[3] = val >> 24;	\
> +	}

Two things about this macro:

  1) you should probably wrap 'val' in parentheses

  2) the use of 'val' 4 times in this macro means it will be evaluated 4
  times; this *can* be OK, except the 'val' parameter as used in context
  is actually a register read (readl()). Is this intentional? Anyway,
  such a construct kinda hides the actual behavior, whether or not it's
  intentional.

To kill off all concerns, perhaps this should be a static inline
function instead. And we might do the same with NFC_BUF_TO_USER_DATA()
then, to match.

Brian

>  
>  #define NFC_DEFAULT_TIMEOUT_MS	1000
>  

[snip rest of patch]

[PATCH 0/2] mtd: nand: sunxi: cleanup

[Boris Brezillon]

Hi Brian,

On Wed, 30 Sep 2015 11:11:35 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Wed, Sep 30, 2015 at 08:36:34AM +0200, Boris Brezillon wrote:
> > On Tue, 29 Sep 2015 15:21:44 -0700
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > 
> > > On Wed, Sep 16, 2015 at 09:46:35AM +0200, Boris Brezillon wrote:
> > > > Hello,
> > > > 
> > > > Those two patches aim at cleaning up the sunxi_nand driver by first adding
> > > > some consistency in the macro definitions, and then factorizing the code
> > > > duplicated code found in hw_ecc and hw_syndrome_ecc implementations.
> > > > 
> > > > Best Regards,
> > > > 
> > > > Boris
> > > 
> > > I believe this series no longer applies cleanly
> > 
> > Hm, actually it depends on fixes you have in your linux-mtd tree and
> > features in your l2-mtd tree.
> > 
> > I don't know how you usually deal with those problems (merging the
> > linux-mtd/master branch into the l2-mtd/master one, or merging
> > last linus' -rc into l2-mtd/master should work), but I'd really
> > like to have this in 4.4 :-).
> 
> linux-mtd.git is prepped for a pull request. I'll do that before the
> week ends. And I'll either merge that back into l2-mtd.git at that time,
> or just pull in v4.3-rc4.

Perfect.

> 
> > I still have the patch using the nand_check_erased_ecc_chunk() function
> > which depends on those 2 patches, and I'd like to have it in 4.4 too.
> > 
> > I know I'm asking a lot, but as explained earlier, I still have a
> > bunch of patches on top of those already posted (mainly to support
> > the hardware randomizer), and I'd like the trivial ones to be merged
> > quickly.
> 
> I'll see what I can apply after pulling together all the latest, but if
> I'm still missing things then, feel free to resend.

Apparently I have a few things to rework in patch 2, so I'll resend
it ;-).

> 
> BTW, I thought there were some things to address on the erased ECC stuff
> still. I'll re-check that series too.

AFAIR, I had a few things to fix, and we were waiting for some feedback.
I also told you I would test the series on an atmel platform (which I
haven't done yet :-/).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/2] mtd: sunxi: nand: refactor ->read_page()/->write_page() code

[Boris Brezillon]

Hi Brian,

On Wed, 30 Sep 2015 11:36:27 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Wed, Sep 16, 2015 at 09:46:37AM +0200, Boris Brezillon wrote:
> > Most of the logic to read/write pages with the HW ECC engine enabled
> > is common to the HW_ECC and NAND_ECC_HW_SYNDROME scheme.
> > Refactor the code to avoid code duplication.
> 
> Hmm, a benign commit description to describe a somewhat complicated
> patch. This seems to do several different types of refactoring all at
> once, and it makes it a bit hard to review. Can you perhaps refactor
> this into 2 or more patches?

Fair enough, I'll try to split it in several pieces.

> e.g., I think the NFC_USER_DATA_TO_BUF()
> stuff can be orthogonal from the introduction of
> sunxi_nfc_hw_ecc_read_chunk() and sunxi_nfc_hw_ecc_read_extra_oob().

Yes, it is, even if I doubt removing this small piece of code can make
the patch more readable ;-).

> 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/sunxi_nand.c | 404 ++++++++++++++++++++++--------------------
> >  1 file changed, 212 insertions(+), 192 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> > index dc44435..640b96c 100644
> > --- a/drivers/mtd/nand/sunxi_nand.c
> > +++ b/drivers/mtd/nand/sunxi_nand.c
> > @@ -159,6 +159,13 @@
> >  /* NFC_USER_DATA helper macros */
> >  #define NFC_BUF_TO_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
> >  					((buf)[2] << 16) | ((buf)[3] << 24))
> > +#define NFC_USER_DATA_TO_BUF(buf, val)	\
> > +	{				\
> > +		(buf)[0] = val;		\
> > +		(buf)[1] = val >> 8;	\
> > +		(buf)[2] = val >> 16;	\
> > +		(buf)[3] = val >> 24;	\
> > +	}
> 
> Two things about this macro:
> 
>   1) you should probably wrap 'val' in parentheses

Yep ...

> 
>   2) the use of 'val' 4 times in this macro means it will be evaluated 4
>   times; this *can* be OK, except the 'val' parameter as used in context
>   is actually a register read (readl()). Is this intentional? Anyway,
>   such a construct kinda hides the actual behavior, whether or not it's
>   intentional.

... and no, it's not intentional.

> 
> To kill off all concerns, perhaps this should be a static inline
> function instead. And we might do the same with NFC_BUF_TO_USER_DATA()
> then, to match.

I like this idea (I'll use lower cases if I convert them to inline
functions).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com