[PATCH v2 0/3] Mediatek MT2712 NAND FLASH Controller driver

[PATCH v2 0/3] Mediatek MT2712 NAND FLASH Controller driver

[Xiaolei Li]

The following patch-set adds support for Mediatek MT2712 NAND FLASH
Controller.

Changes on v2 relative to:
--------------------

tree    : https://github.com/bbrezillon/linux-0day
branch  : nand/next
commit  :
        'commit 2b12c057cc2837312001f4fc3a4d89498fb6f463
        Author: Kamal Dasu <kdasu.kdev@gmail.com>
        Date:   Fri Mar 3 16:16:53 2017 -0500'

Patch v2:
---------
- sperate patches into 3.
- refine DT bindings documentation about NFC and ECC compatible.
- use ecc->devdata->encode_parity_reg0 and ecc->devdata->err_mask
  directly, but hide them in define.

Tests:
------

* ubifs and jffs2 are validated on NAND device MT29F16G08ADBCA
  by 'dd' command.
* all drivers/mtd/tests/* pass.
* speed test:
  eraseblock write speed is 8318 KiB/s
  eraseblock read speed is 11374 KiB/s
  page write speed is 8155 KiB/s
  page read speed is 11290 KiB/s
  2 page write speed is 8217 KiB/s
  2 page read speed is 11354 KiB/s
  erase speed is 116472 KiB/s
  2x multi-block erase speed is 352978 KiB/s
  4x multi-block erase speed is 359750 KiB/s
  8x multi-block erase speed is 361484 KiB/s
  16x multi-block erase speed is 364116 KiB/s
  32x multi-block erase speed is 364116 KiB/s
  64x multi-block erase speed is 364116 KiB/s
Xiaolei Li (3):
  mtd: nand: mediatek: update DT bindings
  mtd: nand: mediatek: add support for different MTK NAND FLASH
    Controller IP
Xiaolei Li (3):
  mtd: nand: mediatek: update DT bindings
  mtd: nand: mediatek: add support for different MTK NAND FLASH
    Controller IP
  mtd: nand: mediatek: add support for MT2712 NAND FLASH Controller

 Documentation/devicetree/bindings/mtd/mtk-nand.txt |   5 +-
 drivers/mtd/nand/mtk_ecc.c                         | 110 +++++++++---
 drivers/mtd/nand/mtk_ecc.h                         |   2 +-
 drivers/mtd/nand/mtk_nand.c                        | 198 ++++++++++++++++-----
 4 files changed, 243 insertions(+), 72 deletions(-)

--
1.9.1

[PATCH v2 1/3] mtd: nand: mediatek: update DT bindings

[Xiaolei Li]

Add MT2712 NAND Flash Controller dt bindings documentation.

Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
---
 Documentation/devicetree/bindings/mtd/mtk-nand.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
index 069c192..dbf9e05 100644
--- a/Documentation/devicetree/bindings/mtd/mtk-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
@@ -12,7 +12,8 @@ tree nodes.
 
 The first part of NFC is NAND Controller Interface (NFI) HW.
 Required NFI properties:
-- compatible:			Should be "mediatek,mtxxxx-nfc".
+- compatible:			Should be one of "mediatek,mt2701-nfc",
+				"mediatek,mt2712-nfc".
 - reg:				Base physical address and size of NFI.
 - interrupts:			Interrupts of NFI.
 - clocks:			NFI required clocks.
@@ -141,7 +142,7 @@ Example:
 ==============
 
 Required BCH properties:
-- compatible:	Should be "mediatek,mtxxxx-ecc".
+- compatible:	Should be one of "mediatek,mt2701-ecc", "mediatek,mt2712-ecc".
 - reg:		Base physical address and size of ECC.
 - interrupts:	Interrupts of ECC.
 - clocks:	ECC required clocks.
-- 
1.9.1

[PATCH v2 2/3] mtd: nand: mediatek: add support for different MTK NAND FLASH Controller IP

[Xiaolei Li]

ECC strength and spare size supported may be different among MTK NAND
FLASH Controller IPs.

This patch contains changes as following:
(1) add new enum mtk_nfc_spare_format to list all spare format.
(2) add new struct mtk_nfc_devdata to support different spare format and
    size.
(3) add new struct mtk_ecc_devdata to support different max ecc strength.
(4) malloc ecc->eccdata buffer according to max ecc strength of this IP.

Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
---
 drivers/mtd/nand/mtk_ecc.c  |  67 ++++++++++++++------
 drivers/mtd/nand/mtk_ecc.h  |   2 +-
 drivers/mtd/nand/mtk_nand.c | 148 ++++++++++++++++++++++++++++++--------------
 3 files changed, 150 insertions(+), 67 deletions(-)

diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
index dbf2562..94d0791 100644
--- a/drivers/mtd/nand/mtk_ecc.c
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -66,7 +66,6 @@
 #define		DEC_CNFG_CORRECT	(0x3 << 12)
 #define ECC_DECIDLE		(0x10C)
 #define ECC_DECENUM0		(0x114)
-#define		ERR_MASK		(0x3f)
 #define ECC_DECDONE		(0x124)
 #define ECC_DECIRQ_EN		(0x200)
 #define ECC_DECIRQ_STA		(0x204)
@@ -78,8 +77,14 @@
 #define ECC_IRQ_REG(op)		((op) == ECC_ENCODE ? \
 					ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
 
+struct mtk_ecc_devdata {
+	u32 err_mask;
+	u8 max_ecc_strength;
+};
+
 struct mtk_ecc {
 	struct device *dev;
+	const struct mtk_ecc_devdata *devdata;
 	void __iomem *regs;
 	struct clk *clk;
 
@@ -87,7 +92,7 @@ struct mtk_ecc {
 	struct mutex lock;
 	u32 sectors;
 
-	u8 eccdata[112];
+	u8 *eccdata;
 };
 
 static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
@@ -247,8 +252,8 @@ void mtk_ecc_get_stats(struct mtk_ecc *ecc, struct mtk_ecc_stats *stats,
 		offset = (i >> 2) << 2;
 		err = readl(ecc->regs + ECC_DECENUM0 + offset);
 		err = err >> ((i % 4) * 8);
-		err &= ERR_MASK;
-		if (err == ERR_MASK) {
+		err &= ecc->devdata->err_mask;
+		if (err == ecc->devdata->err_mask) {
 			/* uncorrectable errors */
 			stats->failed++;
 			continue;
@@ -409,37 +414,68 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
 }
 EXPORT_SYMBOL(mtk_ecc_encode);
 
-void mtk_ecc_adjust_strength(u32 *p)
+void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
 {
-	u32 ecc[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
-			40, 44, 48, 52, 56, 60};
+	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
+				40, 44, 48, 52, 56, 60};
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(ecc); i++) {
-		if (*p <= ecc[i]) {
+	if (*p >= ecc->devdata->max_ecc_strength) {
+		*p = ecc->devdata->max_ecc_strength;
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ecc_level); i++) {
+		if (*p <= ecc_level[i]) {
 			if (!i)
-				*p = ecc[i];
-			else if (*p != ecc[i])
-				*p = ecc[i - 1];
+				*p = ecc_level[i];
+			else if (*p != ecc_level[i])
+				*p = ecc_level[i - 1];
 			return;
 		}
 	}
 
-	*p = ecc[ARRAY_SIZE(ecc) - 1];
+	*p = ecc_level[ARRAY_SIZE(ecc_level) - 1];
 }
 EXPORT_SYMBOL(mtk_ecc_adjust_strength);
 
+static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
+	.err_mask = 0x3f,
+	.max_ecc_strength = 60,
+};
+
+static const struct of_device_id mtk_ecc_dt_match[] = {
+	{
+		.compatible = "mediatek,mt2701-ecc",
+		.data = &mtk_ecc_devdata_mt2701,
+	},
+	{},
+};
+
 static int mtk_ecc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mtk_ecc *ecc;
 	struct resource *res;
+	const struct of_device_id *of_ecc_id = NULL;
+	u32 temp;
 	int irq, ret;
 
 	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
 	if (!ecc)
 		return -ENOMEM;
 
+	of_ecc_id = of_match_device(mtk_ecc_dt_match, &pdev->dev);
+	if (!of_ecc_id)
+		return -ENODEV;
+	ecc->devdata = (struct mtk_ecc_devdata *)of_ecc_id->data;
+
+	temp = (ecc->devdata->max_ecc_strength * ECC_PARITY_BITS + 7) >> 3;
+	temp = round_up(temp, 4);
+	ecc->eccdata = devm_kzalloc(dev, temp, GFP_KERNEL);
+	if (!ecc->eccdata)
+		return -ENOMEM;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ecc->regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(ecc->regs)) {
@@ -508,11 +544,6 @@ static int mtk_ecc_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(mtk_ecc_pm_ops, mtk_ecc_suspend, mtk_ecc_resume);
 #endif
 
-static const struct of_device_id mtk_ecc_dt_match[] = {
-	{ .compatible = "mediatek,mt2701-ecc" },
-	{},
-};
-
 MODULE_DEVICE_TABLE(of, mtk_ecc_dt_match);
 
 static struct platform_driver mtk_ecc_driver = {
diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
index cbeba5c..d245c14 100644
--- a/drivers/mtd/nand/mtk_ecc.h
+++ b/drivers/mtd/nand/mtk_ecc.h
@@ -42,7 +42,7 @@ struct mtk_ecc_config {
 int mtk_ecc_wait_done(struct mtk_ecc *, enum mtk_ecc_operation);
 int mtk_ecc_enable(struct mtk_ecc *, struct mtk_ecc_config *);
 void mtk_ecc_disable(struct mtk_ecc *);
-void mtk_ecc_adjust_strength(u32 *);
+void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p);
 
 struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
 void mtk_ecc_release(struct mtk_ecc *);
diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
index 6c517c6..1a64ea2 100644
--- a/drivers/mtd/nand/mtk_nand.c
+++ b/drivers/mtd/nand/mtk_nand.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/iopoll.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include "mtk_ecc.h"
 
 /* NAND controller register definition */
@@ -38,23 +39,6 @@
 #define NFI_PAGEFMT		(0x04)
 #define		PAGEFMT_FDM_ECC_SHIFT	(12)
 #define		PAGEFMT_FDM_SHIFT	(8)
-#define		PAGEFMT_SPARE_16	(0)
-#define		PAGEFMT_SPARE_26	(1)
-#define		PAGEFMT_SPARE_27	(2)
-#define		PAGEFMT_SPARE_28	(3)
-#define		PAGEFMT_SPARE_32	(4)
-#define		PAGEFMT_SPARE_36	(5)
-#define		PAGEFMT_SPARE_40	(6)
-#define		PAGEFMT_SPARE_44	(7)
-#define		PAGEFMT_SPARE_48	(8)
-#define		PAGEFMT_SPARE_49	(9)
-#define		PAGEFMT_SPARE_50	(0xa)
-#define		PAGEFMT_SPARE_51	(0xb)
-#define		PAGEFMT_SPARE_52	(0xc)
-#define		PAGEFMT_SPARE_62	(0xd)
-#define		PAGEFMT_SPARE_63	(0xe)
-#define		PAGEFMT_SPARE_64	(0xf)
-#define		PAGEFMT_SPARE_SHIFT	(4)
 #define		PAGEFMT_SEC_SEL_512	BIT(2)
 #define		PAGEFMT_512_2K		(0)
 #define		PAGEFMT_2K_4K		(1)
@@ -116,6 +100,11 @@
 #define MTK_MAX_SECTOR		(16)
 #define MTK_NAND_MAX_NSELS	(2)
 
+struct mtk_nfc_devdata {
+	const u32 *spare_format;
+	const u8 *spare_size;
+};
+
 struct mtk_nfc_bad_mark_ctl {
 	void (*bm_swap)(struct mtd_info *, u8 *buf, int raw);
 	u32 sec;
@@ -155,6 +144,7 @@ struct mtk_nfc {
 	struct mtk_ecc *ecc;
 
 	struct device *dev;
+	struct mtk_nfc_devdata *devdata;
 	void __iomem *regs;
 
 	struct completion done;
@@ -163,6 +153,53 @@ struct mtk_nfc {
 	u8 *buffer;
 };
 
+/* NFC Page Format Control Register Spare Size Field Definition */
+enum mtk_nfc_spare_format {
+	PAGEFMT_SPARE_16,
+	PAGEFMT_SPARE_26,
+	PAGEFMT_SPARE_27,
+	PAGEFMT_SPARE_28,
+	PAGEFMT_SPARE_32,
+	PAGEFMT_SPARE_36,
+	PAGEFMT_SPARE_40,
+	PAGEFMT_SPARE_44,
+	PAGEFMT_SPARE_48,
+	PAGEFMT_SPARE_49,
+	PAGEFMT_SPARE_50,
+	PAGEFMT_SPARE_51,
+	PAGEFMT_SPARE_52,
+	PAGEFMT_SPARE_62,
+	PAGEFMT_SPARE_63,
+	PAGEFMT_SPARE_64,
+};
+
+static const u32 mtk_nfc_spare_format_mt2701[] = {
+	[PAGEFMT_SPARE_16]	= (0 << 4),
+	[PAGEFMT_SPARE_26]	= (1 << 4),
+	[PAGEFMT_SPARE_27]	= (2 << 4),
+	[PAGEFMT_SPARE_28]	= (3 << 4),
+	[PAGEFMT_SPARE_32]	= (4 << 4),
+	[PAGEFMT_SPARE_36]	= (5 << 4),
+	[PAGEFMT_SPARE_40]	= (6 << 4),
+	[PAGEFMT_SPARE_44]	= (7 << 4),
+	[PAGEFMT_SPARE_48]	= (8 << 4),
+	[PAGEFMT_SPARE_49]	= (9 << 4),
+	[PAGEFMT_SPARE_50]	= (10 << 4),
+	[PAGEFMT_SPARE_51]	= (11 << 4),
+	[PAGEFMT_SPARE_52]	= (12 << 4),
+	[PAGEFMT_SPARE_62]	= (13 << 4),
+	[PAGEFMT_SPARE_63]	= (14 << 4),
+	[PAGEFMT_SPARE_64]	= (15 << 4),
+};
+
+/*
+ * supported spare size of each IP
+ * 255 is used as ending flag
+ */
+static const u8 spare_size_mt2701[] = {
+	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
+};
+
 static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
 {
 	return container_of(nand, struct mtk_nfc_nand_chip, nand);
@@ -308,6 +345,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
 	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	const u32 *sparefmt = nfc->devdata->spare_format;
 	u32 fmt, spare;
 
 	if (!mtd->writesize)
@@ -354,52 +392,52 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
 
 	switch (spare) {
 	case 16:
-		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_16];
 		break;
 	case 26:
-		fmt |= (PAGEFMT_SPARE_26 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_26];
 		break;
 	case 27:
-		fmt |= (PAGEFMT_SPARE_27 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_27];
 		break;
 	case 28:
-		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_28];
 		break;
 	case 32:
-		fmt |= (PAGEFMT_SPARE_32 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_32];
 		break;
 	case 36:
-		fmt |= (PAGEFMT_SPARE_36 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_36];
 		break;
 	case 40:
-		fmt |= (PAGEFMT_SPARE_40 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_40];
 		break;
 	case 44:
-		fmt |= (PAGEFMT_SPARE_44 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_44];
 		break;
 	case 48:
-		fmt |= (PAGEFMT_SPARE_48 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_48];
 		break;
 	case 49:
-		fmt |= (PAGEFMT_SPARE_49 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_49];
 		break;
 	case 50:
-		fmt |= (PAGEFMT_SPARE_50 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_50];
 		break;
 	case 51:
-		fmt |= (PAGEFMT_SPARE_51 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_51];
 		break;
 	case 52:
-		fmt |= (PAGEFMT_SPARE_52 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_52];
 		break;
 	case 62:
-		fmt |= (PAGEFMT_SPARE_62 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_62];
 		break;
 	case 63:
-		fmt |= (PAGEFMT_SPARE_63 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_63];
 		break;
 	case 64:
-		fmt |= (PAGEFMT_SPARE_64 << PAGEFMT_SPARE_SHIFT);
+		fmt |= sparefmt[PAGEFMT_SPARE_64];
 		break;
 	default:
 		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
@@ -408,7 +446,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
 
 	fmt |= mtk_nand->fdm.reg_size << PAGEFMT_FDM_SHIFT;
 	fmt |= mtk_nand->fdm.ecc_size << PAGEFMT_FDM_ECC_SHIFT;
-	nfi_writew(nfc, fmt, NFI_PAGEFMT);
+	nfi_writel(nfc, fmt, NFI_PAGEFMT);
 
 	nfc->ecc_cfg.strength = chip->ecc.strength;
 	nfc->ecc_cfg.len = chip->ecc.size + mtk_nand->fdm.ecc_size;
@@ -1009,7 +1047,7 @@ static inline void mtk_nfc_hw_init(struct mtk_nfc *nfc)
 	 * 0  : poll the status of the busy/ready signal after [7:4]*16 cycles.
 	 */
 	nfi_writew(nfc, 0xf1, NFI_CNRNB);
-	nfi_writew(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
+	nfi_writel(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
 
 	mtk_nfc_hw_reset(nfc);
 
@@ -1134,8 +1172,8 @@ static void mtk_nfc_set_bad_mark_ctl(struct mtk_nfc_bad_mark_ctl *bm_ctl,
 static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
 {
 	struct nand_chip *nand = mtd_to_nand(mtd);
-	u32 spare[] = {16, 26, 27, 28, 32, 36, 40, 44,
-			48, 49, 50, 51, 52, 62, 63, 64};
+	struct mtk_nfc *nfc = nand_get_controller_data(nand);
+	const u8 *spare = nfc->devdata->spare_size;
 	u32 eccsteps, i;
 
 	eccsteps = mtd->writesize / nand->ecc.size;
@@ -1144,7 +1182,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
 	if (nand->ecc.size == 1024)
 		*sps >>= 1;
 
-	for (i = 0; i < ARRAY_SIZE(spare); i++) {
+	for (i = 0; ; i++) {
 		if (*sps <= spare[i]) {
 			if (!i)
 				*sps = spare[i];
@@ -1154,9 +1192,6 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
 		}
 	}
 
-	if (i >= ARRAY_SIZE(spare))
-		*sps = spare[ARRAY_SIZE(spare) - 1];
-
 	if (nand->ecc.size == 1024)
 		*sps <<= 1;
 }
@@ -1164,6 +1199,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
 static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
 {
 	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(nand);
 	u32 spare;
 	int free;
 
@@ -1214,7 +1250,7 @@ static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
 		}
 	}
 
-	mtk_ecc_adjust_strength(&nand->ecc.strength);
+	mtk_ecc_adjust_strength(nfc->ecc, &nand->ecc.strength);
 
 	dev_info(dev, "eccsize %d eccstrength %d\n",
 		 nand->ecc.size, nand->ecc.strength);
@@ -1354,12 +1390,27 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
 	return 0;
 }
 
+static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2701 = {
+	.spare_format = mtk_nfc_spare_format_mt2701,
+	.spare_size = spare_size_mt2701,
+};
+
+static const struct of_device_id mtk_nfc_id_table[] = {
+	{
+		.compatible = "mediatek,mt2701-nfc",
+		.data = &mtk_nfc_devdata_mt2701,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
+
 static int mtk_nfc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct mtk_nfc *nfc;
 	struct resource *res;
+	const struct of_device_id *of_nfc_id = NULL;
 	int ret, irq;
 
 	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
@@ -1423,6 +1474,13 @@ static int mtk_nfc_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	of_nfc_id = of_match_device(mtk_nfc_id_table, &pdev->dev);
+	if (!of_nfc_id) {
+		ret = -ENODEV;
+		goto clk_disable;
+	}
+	nfc->devdata = (struct mtk_nfc_devdata *)of_nfc_id->data;
+
 	platform_set_drvdata(pdev, nfc);
 
 	ret = mtk_nfc_nand_chips_init(dev, nfc);
@@ -1503,12 +1561,6 @@ static int mtk_nfc_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(mtk_nfc_pm_ops, mtk_nfc_suspend, mtk_nfc_resume);
 #endif
 
-static const struct of_device_id mtk_nfc_id_table[] = {
-	{ .compatible = "mediatek,mt2701-nfc" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
-
 static struct platform_driver mtk_nfc_driver = {
 	.probe  = mtk_nfc_probe,
 	.remove = mtk_nfc_remove,
-- 
1.9.1

Re: [PATCH v2 2/3] mtd: nand: mediatek: add support for different MTK NAND FLASH Controller IP

[Boris Brezillon]

On Fri, 12 May 2017 12:33:22 +0800
Xiaolei Li <xiaolei.li@mediatek.com> wrote:

> ECC strength and spare size supported may be different among MTK NAND
> FLASH Controller IPs.
> 
> This patch contains changes as following:
> (1) add new enum mtk_nfc_spare_format to list all spare format.
> (2) add new struct mtk_nfc_devdata to support different spare format and
>     size.
> (3) add new struct mtk_ecc_devdata to support different max ecc strength.
> (4) malloc ecc->eccdata buffer according to max ecc strength of this IP.
> 
> Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
> ---
>  drivers/mtd/nand/mtk_ecc.c  |  67 ++++++++++++++------
>  drivers/mtd/nand/mtk_ecc.h  |   2 +-
>  drivers/mtd/nand/mtk_nand.c | 148 ++++++++++++++++++++++++++++++--------------
>  3 files changed, 150 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> index dbf2562..94d0791 100644
> --- a/drivers/mtd/nand/mtk_ecc.c
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -66,7 +66,6 @@
>  #define		DEC_CNFG_CORRECT	(0x3 << 12)
>  #define ECC_DECIDLE		(0x10C)
>  #define ECC_DECENUM0		(0x114)
> -#define		ERR_MASK		(0x3f)
>  #define ECC_DECDONE		(0x124)
>  #define ECC_DECIRQ_EN		(0x200)
>  #define ECC_DECIRQ_STA		(0x204)
> @@ -78,8 +77,14 @@
>  #define ECC_IRQ_REG(op)		((op) == ECC_ENCODE ? \
>  					ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
>  
> +struct mtk_ecc_devdata {

I usually prefer a _caps suffix (instead of _devdata) for this kind of
per-IP capability variations, but that's not a strong requirement.

> +	u32 err_mask;
> +	u8 max_ecc_strength;
> +};
> +
>  struct mtk_ecc {
>  	struct device *dev;
> +	const struct mtk_ecc_devdata *devdata;
>  	void __iomem *regs;
>  	struct clk *clk;
>  
> @@ -87,7 +92,7 @@ struct mtk_ecc {
>  	struct mutex lock;
>  	u32 sectors;
>  
> -	u8 eccdata[112];
> +	u8 *eccdata;
>  };
>  
>  static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> @@ -247,8 +252,8 @@ void mtk_ecc_get_stats(struct mtk_ecc *ecc, struct mtk_ecc_stats *stats,
>  		offset = (i >> 2) << 2;
>  		err = readl(ecc->regs + ECC_DECENUM0 + offset);
>  		err = err >> ((i % 4) * 8);
> -		err &= ERR_MASK;
> -		if (err == ERR_MASK) {
> +		err &= ecc->devdata->err_mask;
> +		if (err == ecc->devdata->err_mask) {
>  			/* uncorrectable errors */
>  			stats->failed++;
>  			continue;
> @@ -409,37 +414,68 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>  }
>  EXPORT_SYMBOL(mtk_ecc_encode);
>  
> -void mtk_ecc_adjust_strength(u32 *p)
> +void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
>  {
> -	u32 ecc[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> -			40, 44, 48, 52, 56, 60};
> +	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> +				40, 44, 48, 52, 56, 60};

Can we rename this variable strength or ecc_strength. It should also
probably be a static const. Also, if some variation do not support the
whole set of strengths, it would be safer to have the strengths array
(and its size) directly stored in mtd_ecc_devdata.

>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(ecc); i++) {
> -		if (*p <= ecc[i]) {
> +	if (*p >= ecc->devdata->max_ecc_strength) {
> +		*p = ecc->devdata->max_ecc_strength;
> +		return;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ecc_level); i++) {
> +		if (*p <= ecc_level[i]) {
>  			if (!i)
> -				*p = ecc[i];
> -			else if (*p != ecc[i])
> -				*p = ecc[i - 1];
> +				*p = ecc_level[i];
> +			else if (*p != ecc_level[i])
> +				*p = ecc_level[i - 1];

I know I initially reviewed and merge this driver, but I really don't
understand why we decide to take a weaker ECC if the strength does not
exactly match what was requested. It should be the opposite: take a
stronger ECC config, unless ECC bytes do not fit in the OOB area.

>  			return;
>  		}
>  	}
>  
> -	*p = ecc[ARRAY_SIZE(ecc) - 1];
> +	*p = ecc_level[ARRAY_SIZE(ecc_level) - 1];
>  }
>  EXPORT_SYMBOL(mtk_ecc_adjust_strength);
>  
> +static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
> +	.err_mask = 0x3f,
> +	.max_ecc_strength = 60,
> +};
> +
> +static const struct of_device_id mtk_ecc_dt_match[] = {
> +	{
> +		.compatible = "mediatek,mt2701-ecc",
> +		.data = &mtk_ecc_devdata_mt2701,
> +	},
> +	{},
> +};
> +
>  static int mtk_ecc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct mtk_ecc *ecc;
>  	struct resource *res;
> +	const struct of_device_id *of_ecc_id = NULL;
> +	u32 temp;

Please rename max_eccdata_size, or something more precise than temp.
temp is acceptable when the variable is used several times to calculate
various things. That's not the case here.

>  	int irq, ret;
>  
>  	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
>  	if (!ecc)
>  		return -ENOMEM;
>  
> +	of_ecc_id = of_match_device(mtk_ecc_dt_match, &pdev->dev);
> +	if (!of_ecc_id)
> +		return -ENODEV;

Blank line please.

> +	ecc->devdata = (struct mtk_ecc_devdata *)of_ecc_id->data;

The cast is unneeded.

> +
> +	temp = (ecc->devdata->max_ecc_strength * ECC_PARITY_BITS + 7) >> 3;
> +	temp = round_up(temp, 4);
> +	ecc->eccdata = devm_kzalloc(dev, temp, GFP_KERNEL);
> +	if (!ecc->eccdata)
> +		return -ENOMEM;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ecc->regs = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(ecc->regs)) {
> @@ -508,11 +544,6 @@ static int mtk_ecc_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(mtk_ecc_pm_ops, mtk_ecc_suspend, mtk_ecc_resume);
>  #endif
>  
> -static const struct of_device_id mtk_ecc_dt_match[] = {
> -	{ .compatible = "mediatek,mt2701-ecc" },
> -	{},
> -};
> -
>  MODULE_DEVICE_TABLE(of, mtk_ecc_dt_match);
>  
>  static struct platform_driver mtk_ecc_driver = {
> diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
> index cbeba5c..d245c14 100644
> --- a/drivers/mtd/nand/mtk_ecc.h
> +++ b/drivers/mtd/nand/mtk_ecc.h
> @@ -42,7 +42,7 @@ struct mtk_ecc_config {
>  int mtk_ecc_wait_done(struct mtk_ecc *, enum mtk_ecc_operation);
>  int mtk_ecc_enable(struct mtk_ecc *, struct mtk_ecc_config *);
>  void mtk_ecc_disable(struct mtk_ecc *);
> -void mtk_ecc_adjust_strength(u32 *);
> +void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p);
>  
>  struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
>  void mtk_ecc_release(struct mtk_ecc *);
> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> index 6c517c6..1a64ea2 100644
> --- a/drivers/mtd/nand/mtk_nand.c
> +++ b/drivers/mtd/nand/mtk_nand.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/iopoll.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include "mtk_ecc.h"
>  
>  /* NAND controller register definition */
> @@ -38,23 +39,6 @@
>  #define NFI_PAGEFMT		(0x04)
>  #define		PAGEFMT_FDM_ECC_SHIFT	(12)
>  #define		PAGEFMT_FDM_SHIFT	(8)
> -#define		PAGEFMT_SPARE_16	(0)
> -#define		PAGEFMT_SPARE_26	(1)
> -#define		PAGEFMT_SPARE_27	(2)
> -#define		PAGEFMT_SPARE_28	(3)
> -#define		PAGEFMT_SPARE_32	(4)
> -#define		PAGEFMT_SPARE_36	(5)
> -#define		PAGEFMT_SPARE_40	(6)
> -#define		PAGEFMT_SPARE_44	(7)
> -#define		PAGEFMT_SPARE_48	(8)
> -#define		PAGEFMT_SPARE_49	(9)
> -#define		PAGEFMT_SPARE_50	(0xa)
> -#define		PAGEFMT_SPARE_51	(0xb)
> -#define		PAGEFMT_SPARE_52	(0xc)
> -#define		PAGEFMT_SPARE_62	(0xd)
> -#define		PAGEFMT_SPARE_63	(0xe)
> -#define		PAGEFMT_SPARE_64	(0xf)
> -#define		PAGEFMT_SPARE_SHIFT	(4)
>  #define		PAGEFMT_SEC_SEL_512	BIT(2)
>  #define		PAGEFMT_512_2K		(0)
>  #define		PAGEFMT_2K_4K		(1)
> @@ -116,6 +100,11 @@
>  #define MTK_MAX_SECTOR		(16)
>  #define MTK_NAND_MAX_NSELS	(2)
>  
> +struct mtk_nfc_devdata {
> +	const u32 *spare_format;
> +	const u8 *spare_size;
> +};
> +
>  struct mtk_nfc_bad_mark_ctl {
>  	void (*bm_swap)(struct mtd_info *, u8 *buf, int raw);
>  	u32 sec;
> @@ -155,6 +144,7 @@ struct mtk_nfc {
>  	struct mtk_ecc *ecc;
>  
>  	struct device *dev;
> +	struct mtk_nfc_devdata *devdata;
>  	void __iomem *regs;
>  
>  	struct completion done;
> @@ -163,6 +153,53 @@ struct mtk_nfc {
>  	u8 *buffer;
>  };
>  
> +/* NFC Page Format Control Register Spare Size Field Definition */
> +enum mtk_nfc_spare_format {
> +	PAGEFMT_SPARE_16,
> +	PAGEFMT_SPARE_26,
> +	PAGEFMT_SPARE_27,
> +	PAGEFMT_SPARE_28,
> +	PAGEFMT_SPARE_32,
> +	PAGEFMT_SPARE_36,
> +	PAGEFMT_SPARE_40,
> +	PAGEFMT_SPARE_44,
> +	PAGEFMT_SPARE_48,
> +	PAGEFMT_SPARE_49,
> +	PAGEFMT_SPARE_50,
> +	PAGEFMT_SPARE_51,
> +	PAGEFMT_SPARE_52,
> +	PAGEFMT_SPARE_62,
> +	PAGEFMT_SPARE_63,
> +	PAGEFMT_SPARE_64,
> +};
> +
> +static const u32 mtk_nfc_spare_format_mt2701[] = {
> +	[PAGEFMT_SPARE_16]	= (0 << 4),
> +	[PAGEFMT_SPARE_26]	= (1 << 4),
> +	[PAGEFMT_SPARE_27]	= (2 << 4),
> +	[PAGEFMT_SPARE_28]	= (3 << 4),
> +	[PAGEFMT_SPARE_32]	= (4 << 4),
> +	[PAGEFMT_SPARE_36]	= (5 << 4),
> +	[PAGEFMT_SPARE_40]	= (6 << 4),
> +	[PAGEFMT_SPARE_44]	= (7 << 4),
> +	[PAGEFMT_SPARE_48]	= (8 << 4),
> +	[PAGEFMT_SPARE_49]	= (9 << 4),
> +	[PAGEFMT_SPARE_50]	= (10 << 4),
> +	[PAGEFMT_SPARE_51]	= (11 << 4),
> +	[PAGEFMT_SPARE_52]	= (12 << 4),
> +	[PAGEFMT_SPARE_62]	= (13 << 4),
> +	[PAGEFMT_SPARE_63]	= (14 << 4),
> +	[PAGEFMT_SPARE_64]	= (15 << 4),
> +};
> +
> +/*
> + * supported spare size of each IP
> + * 255 is used as ending flag
> + */
> +static const u8 spare_size_mt2701[] = {
> +	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
> +};

I guess 255 (or 0xff) is a sentinel. That's probably easier to store
the array size along with the spare_size array in mtk_nfc_devdata.

> +
>  static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
>  {
>  	return container_of(nand, struct mtk_nfc_nand_chip, nand);
> @@ -308,6 +345,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
>  	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	const u32 *sparefmt = nfc->devdata->spare_format;
>  	u32 fmt, spare;
>  
>  	if (!mtd->writesize)
> @@ -354,52 +392,52 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
>  
>  	switch (spare) {
>  	case 16:
> -		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_16];
>  		break;
>  	case 26:
> -		fmt |= (PAGEFMT_SPARE_26 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_26];
>  		break;
>  	case 27:
> -		fmt |= (PAGEFMT_SPARE_27 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_27];
>  		break;
>  	case 28:
> -		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_28];
>  		break;
>  	case 32:
> -		fmt |= (PAGEFMT_SPARE_32 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_32];
>  		break;
>  	case 36:
> -		fmt |= (PAGEFMT_SPARE_36 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_36];
>  		break;
>  	case 40:
> -		fmt |= (PAGEFMT_SPARE_40 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_40];
>  		break;
>  	case 44:
> -		fmt |= (PAGEFMT_SPARE_44 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_44];
>  		break;
>  	case 48:
> -		fmt |= (PAGEFMT_SPARE_48 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_48];
>  		break;
>  	case 49:
> -		fmt |= (PAGEFMT_SPARE_49 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_49];
>  		break;
>  	case 50:
> -		fmt |= (PAGEFMT_SPARE_50 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_50];
>  		break;
>  	case 51:
> -		fmt |= (PAGEFMT_SPARE_51 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_51];
>  		break;
>  	case 52:
> -		fmt |= (PAGEFMT_SPARE_52 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_52];
>  		break;
>  	case 62:
> -		fmt |= (PAGEFMT_SPARE_62 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_62];
>  		break;
>  	case 63:
> -		fmt |= (PAGEFMT_SPARE_63 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_63];
>  		break;
>  	case 64:
> -		fmt |= (PAGEFMT_SPARE_64 << PAGEFMT_SPARE_SHIFT);
> +		fmt |= sparefmt[PAGEFMT_SPARE_64];
>  		break;

Could probably be turned into a for loop:

	for (i = 0; i < nfc->devdata->num_spare_format; i++) {
		if (nfc->devdata->spare_size[i] == spare)
			break;
	}

	if (i == nfc->devdata->num_spare_format) {
		dev_err()
		return -EINVAL;
	}

	fmt |= sparefmt[i];

>  	default:
>  		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
> @@ -408,7 +446,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
>  
>  	fmt |= mtk_nand->fdm.reg_size << PAGEFMT_FDM_SHIFT;
>  	fmt |= mtk_nand->fdm.ecc_size << PAGEFMT_FDM_ECC_SHIFT;
> -	nfi_writew(nfc, fmt, NFI_PAGEFMT);
> +	nfi_writel(nfc, fmt, NFI_PAGEFMT);

Is this still compatible with the old IP? Maybe this should go in a
separate patch. 

>  
>  	nfc->ecc_cfg.strength = chip->ecc.strength;
>  	nfc->ecc_cfg.len = chip->ecc.size + mtk_nand->fdm.ecc_size;
> @@ -1009,7 +1047,7 @@ static inline void mtk_nfc_hw_init(struct mtk_nfc *nfc)
>  	 * 0  : poll the status of the busy/ready signal after [7:4]*16 cycles.
>  	 */
>  	nfi_writew(nfc, 0xf1, NFI_CNRNB);
> -	nfi_writew(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
> +	nfi_writel(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
>  
>  	mtk_nfc_hw_reset(nfc);
>  
> @@ -1134,8 +1172,8 @@ static void mtk_nfc_set_bad_mark_ctl(struct mtk_nfc_bad_mark_ctl *bm_ctl,
>  static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
>  {
>  	struct nand_chip *nand = mtd_to_nand(mtd);
> -	u32 spare[] = {16, 26, 27, 28, 32, 36, 40, 44,
> -			48, 49, 50, 51, 52, 62, 63, 64};
> +	struct mtk_nfc *nfc = nand_get_controller_data(nand);
> +	const u8 *spare = nfc->devdata->spare_size;
>  	u32 eccsteps, i;
>  
>  	eccsteps = mtd->writesize / nand->ecc.size;
> @@ -1144,7 +1182,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
>  	if (nand->ecc.size == 1024)
>  		*sps >>= 1;
>  
> -	for (i = 0; i < ARRAY_SIZE(spare); i++) {
> +	for (i = 0; ; i++) {

With the array size in devdata you could check that i is less than
num_spare_format.
BTW, I don't see any check on the sentinel val.

>  		if (*sps <= spare[i]) {
>  			if (!i)
>  				*sps = spare[i];
> @@ -1154,9 +1192,6 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
>  		}
>  	}
>  
> -	if (i >= ARRAY_SIZE(spare))
> -		*sps = spare[ARRAY_SIZE(spare) - 1];
> -
>  	if (nand->ecc.size == 1024)
>  		*sps <<= 1;
>  }
> @@ -1164,6 +1199,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
>  static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
>  {
>  	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct mtk_nfc *nfc = nand_get_controller_data(nand);
>  	u32 spare;
>  	int free;
>  
> @@ -1214,7 +1250,7 @@ static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
>  		}
>  	}
>  
> -	mtk_ecc_adjust_strength(&nand->ecc.strength);
> +	mtk_ecc_adjust_strength(nfc->ecc, &nand->ecc.strength);
>  
>  	dev_info(dev, "eccsize %d eccstrength %d\n",
>  		 nand->ecc.size, nand->ecc.strength);
> @@ -1354,12 +1390,27 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
>  	return 0;
>  }
>  
> +static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2701 = {
> +	.spare_format = mtk_nfc_spare_format_mt2701,
> +	.spare_size = spare_size_mt2701,
> +};
> +
> +static const struct of_device_id mtk_nfc_id_table[] = {
> +	{
> +		.compatible = "mediatek,mt2701-nfc",
> +		.data = &mtk_nfc_devdata_mt2701,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
> +
>  static int mtk_nfc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct mtk_nfc *nfc;
>  	struct resource *res;
> +	const struct of_device_id *of_nfc_id = NULL;
>  	int ret, irq;
>  
>  	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> @@ -1423,6 +1474,13 @@ static int mtk_nfc_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	of_nfc_id = of_match_device(mtk_nfc_id_table, &pdev->dev);
> +	if (!of_nfc_id) {
> +		ret = -ENODEV;
> +		goto clk_disable;
> +	}

Blank line.

> +	nfc->devdata = (struct mtk_nfc_devdata *)of_nfc_id->data;

Cast unneeded.

> +
>  	platform_set_drvdata(pdev, nfc);
>  
>  	ret = mtk_nfc_nand_chips_init(dev, nfc);
> @@ -1503,12 +1561,6 @@ static int mtk_nfc_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(mtk_nfc_pm_ops, mtk_nfc_suspend, mtk_nfc_resume);
>  #endif
>  
> -static const struct of_device_id mtk_nfc_id_table[] = {
> -	{ .compatible = "mediatek,mt2701-nfc" },
> -	{}
> -};
> -MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
> -
>  static struct platform_driver mtk_nfc_driver = {
>  	.probe  = mtk_nfc_probe,
>  	.remove = mtk_nfc_remove,

Re: [PATCH v2 2/3] mtd: nand: mediatek: add support for different MTK NAND FLASH Controller IP

[xiaolei li]

On Fri, 2017-05-12 at 08:44 +0200, Boris Brezillon wrote:
> On Fri, 12 May 2017 12:33:22 +0800
> Xiaolei Li <xiaolei.li@mediatek.com> wrote:
> 
> > ECC strength and spare size supported may be different among MTK NAND
> > FLASH Controller IPs.
> > 
> > This patch contains changes as following:
> > (1) add new enum mtk_nfc_spare_format to list all spare format.
> > (2) add new struct mtk_nfc_devdata to support different spare format and
> >     size.
> > (3) add new struct mtk_ecc_devdata to support different max ecc strength.
> > (4) malloc ecc->eccdata buffer according to max ecc strength of this IP.
> > 
> > Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
> > ---
> >  drivers/mtd/nand/mtk_ecc.c  |  67 ++++++++++++++------
> >  drivers/mtd/nand/mtk_ecc.h  |   2 +-
> >  drivers/mtd/nand/mtk_nand.c | 148 ++++++++++++++++++++++++++++++--------------
> >  3 files changed, 150 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> > index dbf2562..94d0791 100644
> > --- a/drivers/mtd/nand/mtk_ecc.c
> > +++ b/drivers/mtd/nand/mtk_ecc.c
> > @@ -66,7 +66,6 @@
> >  #define		DEC_CNFG_CORRECT	(0x3 << 12)
> >  #define ECC_DECIDLE		(0x10C)
> >  #define ECC_DECENUM0		(0x114)
> > -#define		ERR_MASK		(0x3f)
> >  #define ECC_DECDONE		(0x124)
> >  #define ECC_DECIRQ_EN		(0x200)
> >  #define ECC_DECIRQ_STA		(0x204)
> > @@ -78,8 +77,14 @@
> >  #define ECC_IRQ_REG(op)		((op) == ECC_ENCODE ? \
> >  					ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
> >  
> > +struct mtk_ecc_devdata {
> 
> I usually prefer a _caps suffix (instead of _devdata) for this kind of
> per-IP capability variations, but that's not a strong requirement.
> 
'_caps' is OK for me. will change to mtk_ecc_devdata to mtk_ecc_caps
next version.

Also will change mtk_nfc_devdata later.

> > +	u32 err_mask;
> > +	u8 max_ecc_strength;
> > +};
> > +
> >  struct mtk_ecc {
> >  	struct device *dev;
> > +	const struct mtk_ecc_devdata *devdata;
> >  	void __iomem *regs;
> >  	struct clk *clk;
> >  
> > @@ -87,7 +92,7 @@ struct mtk_ecc {
> >  	struct mutex lock;
> >  	u32 sectors;
> >  
> > -	u8 eccdata[112];
> > +	u8 *eccdata;
> >  };
> >  
> >  static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> > @@ -247,8 +252,8 @@ void mtk_ecc_get_stats(struct mtk_ecc *ecc, struct mtk_ecc_stats *stats,
> >  		offset = (i >> 2) << 2;
> >  		err = readl(ecc->regs + ECC_DECENUM0 + offset);
> >  		err = err >> ((i % 4) * 8);
> > -		err &= ERR_MASK;
> > -		if (err == ERR_MASK) {
> > +		err &= ecc->devdata->err_mask;
> > +		if (err == ecc->devdata->err_mask) {
> >  			/* uncorrectable errors */
> >  			stats->failed++;
> >  			continue;
> > @@ -409,37 +414,68 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> >  }
> >  EXPORT_SYMBOL(mtk_ecc_encode);
> >  
> > -void mtk_ecc_adjust_strength(u32 *p)
> > +void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
> >  {
> > -	u32 ecc[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> > -			40, 44, 48, 52, 56, 60};
> > +	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> > +				40, 44, 48, 52, 56, 60};
> 
> Can we rename this variable strength or ecc_strength. It should also
> probably be a static const. Also, if some variation do not support the
> whole set of strengths, it would be safer to have the strengths array
> (and its size) directly stored in mtd_ecc_devdata.
OK.

> 
> >  	int i;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(ecc); i++) {
> > -		if (*p <= ecc[i]) {
> > +	if (*p >= ecc->devdata->max_ecc_strength) {
> > +		*p = ecc->devdata->max_ecc_strength;
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ecc_level); i++) {
> > +		if (*p <= ecc_level[i]) {
> >  			if (!i)
> > -				*p = ecc[i];
> > -			else if (*p != ecc[i])
> > -				*p = ecc[i - 1];
> > +				*p = ecc_level[i];
> > +			else if (*p != ecc_level[i])
> > +				*p = ecc_level[i - 1];
> 
> I know I initially reviewed and merge this driver, but I really don't
> understand why we decide to take a weaker ECC if the strength does not
> exactly match what was requested. It should be the opposite: take a
> stronger ECC config, unless ECC bytes do not fit in the OOB area.
I got what you mean.

The reason why take a weaker ECC is that our ECC HW just supports fixed
ecc strength it provides, like the register define bellow:

Register NFIECC_ENCCNFG bit[0:4] stands for ecc strength supported. 
5'd0:means the NFIECC is capable of correct 4 bits in one block size.
5'd1:means the NFIECC is capable of correct 6 bits in one block size.
5'd2:means the NFIECC is capable of correct 8 bits in one block size.
5'd3:means the NFIECC is capable of correct 10 bits in one block size.
5'd4:means the NFIECC is capable of correct 12 bits in one block size.
5'd5:means the NFIECC is capable of correct 14 bits in one block size.
5'd6:means the NFIECC is capable of correct 16 bits in one block size.
5'd7:means the NFIECC is capable of correct 18 bits in one block size.
5'd8:means the NFIECC is capable of correct 20 bits in one block size.
5'd9:means the NFIECC is capable of correct 22 bits in one block size.
5'd10:means the NFIECC is capable of correct 24 bits in one block size.
5'd11:means the NFIECC is capable of correct 28 bits in one block size.
5'd12:means the NFIECC is capable of correct 32 bits in one block size.
5'd13:means the NFIECC is capable of correct 36 bits in one block size.
5'd14:means the NFIECC is capable of correct 40 bits in one block size.
5'd15:means the NFIECC is capable of correct 44 bits in one block size.
5'd16:means the NFIECC is capable of correct 48 bits in one block size.
5'd17:means the NFIECC is capable of correct 52 bits in one block size.
5'd18:means the NFIECC is capable of correct 56 bits in one block size.
5'd19:means the NFIECC is capable of correct 60 bits in one block size.
5'd20:means the NFIECC is capable of correct 68 bits in one block size.
5'd21:means the NFIECC is capable of correct 72 bits in one block size.
5'd22:means the NFIECC is capable of correct 80 bits in one block size.

Actually, we have tried to enhance ECC strength in function
mtk_nfc_ecc_init of file mtk_nand.c if OOB is enough to store ecc
strength spec required.
So, if OOB is enough, ecc strength here is already stronger than spec
required.

> 
> >  			return;
> >  		}
> >  	}
> >  
> > -	*p = ecc[ARRAY_SIZE(ecc) - 1];
> > +	*p = ecc_level[ARRAY_SIZE(ecc_level) - 1];
> >  }
> >  EXPORT_SYMBOL(mtk_ecc_adjust_strength);
> >  
> > +static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
> > +	.err_mask = 0x3f,
> > +	.max_ecc_strength = 60,
> > +};
> > +
> > +static const struct of_device_id mtk_ecc_dt_match[] = {
> > +	{
> > +		.compatible = "mediatek,mt2701-ecc",
> > +		.data = &mtk_ecc_devdata_mt2701,
> > +	},
> > +	{},
> > +};
> > +
> >  static int mtk_ecc_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct mtk_ecc *ecc;
> >  	struct resource *res;
> > +	const struct of_device_id *of_ecc_id = NULL;
> > +	u32 temp;
> 
> Please rename max_eccdata_size, or something more precise than temp.
> temp is acceptable when the variable is used several times to calculate
> various things. That's not the case here.
OK.

> 
> >  	int irq, ret;
> >  
> >  	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> >  	if (!ecc)
> >  		return -ENOMEM;
> >  
> > +	of_ecc_id = of_match_device(mtk_ecc_dt_match, &pdev->dev);
> > +	if (!of_ecc_id)
> > +		return -ENODEV;
> 
> Blank line please.

OK.
> 
> > +	ecc->devdata = (struct mtk_ecc_devdata *)of_ecc_id->data;
> 
> The cast is unneeded.
> 
OK.

> > +
> > +	temp = (ecc->devdata->max_ecc_strength * ECC_PARITY_BITS + 7) >> 3;
> > +	temp = round_up(temp, 4);
> > +	ecc->eccdata = devm_kzalloc(dev, temp, GFP_KERNEL);
> > +	if (!ecc->eccdata)
> > +		return -ENOMEM;
> > +
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	ecc->regs = devm_ioremap_resource(dev, res);
> >  	if (IS_ERR(ecc->regs)) {
> > @@ -508,11 +544,6 @@ static int mtk_ecc_resume(struct device *dev)
> >  static SIMPLE_DEV_PM_OPS(mtk_ecc_pm_ops, mtk_ecc_suspend, mtk_ecc_resume);
> >  #endif
> >  
> > -static const struct of_device_id mtk_ecc_dt_match[] = {
> > -	{ .compatible = "mediatek,mt2701-ecc" },
> > -	{},
> > -};
> > -
> >  MODULE_DEVICE_TABLE(of, mtk_ecc_dt_match);
> >  
> >  static struct platform_driver mtk_ecc_driver = {
> > diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
> > index cbeba5c..d245c14 100644
> > --- a/drivers/mtd/nand/mtk_ecc.h
> > +++ b/drivers/mtd/nand/mtk_ecc.h
> > @@ -42,7 +42,7 @@ struct mtk_ecc_config {
> >  int mtk_ecc_wait_done(struct mtk_ecc *, enum mtk_ecc_operation);
> >  int mtk_ecc_enable(struct mtk_ecc *, struct mtk_ecc_config *);
> >  void mtk_ecc_disable(struct mtk_ecc *);
> > -void mtk_ecc_adjust_strength(u32 *);
> > +void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p);
> >  
> >  struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
> >  void mtk_ecc_release(struct mtk_ecc *);
> > diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> > index 6c517c6..1a64ea2 100644
> > --- a/drivers/mtd/nand/mtk_nand.c
> > +++ b/drivers/mtd/nand/mtk_nand.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/module.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include "mtk_ecc.h"
> >  
> >  /* NAND controller register definition */
> > @@ -38,23 +39,6 @@
> >  #define NFI_PAGEFMT		(0x04)
> >  #define		PAGEFMT_FDM_ECC_SHIFT	(12)
> >  #define		PAGEFMT_FDM_SHIFT	(8)
> > -#define		PAGEFMT_SPARE_16	(0)
> > -#define		PAGEFMT_SPARE_26	(1)
> > -#define		PAGEFMT_SPARE_27	(2)
> > -#define		PAGEFMT_SPARE_28	(3)
> > -#define		PAGEFMT_SPARE_32	(4)
> > -#define		PAGEFMT_SPARE_36	(5)
> > -#define		PAGEFMT_SPARE_40	(6)
> > -#define		PAGEFMT_SPARE_44	(7)
> > -#define		PAGEFMT_SPARE_48	(8)
> > -#define		PAGEFMT_SPARE_49	(9)
> > -#define		PAGEFMT_SPARE_50	(0xa)
> > -#define		PAGEFMT_SPARE_51	(0xb)
> > -#define		PAGEFMT_SPARE_52	(0xc)
> > -#define		PAGEFMT_SPARE_62	(0xd)
> > -#define		PAGEFMT_SPARE_63	(0xe)
> > -#define		PAGEFMT_SPARE_64	(0xf)
> > -#define		PAGEFMT_SPARE_SHIFT	(4)
> >  #define		PAGEFMT_SEC_SEL_512	BIT(2)
> >  #define		PAGEFMT_512_2K		(0)
> >  #define		PAGEFMT_2K_4K		(1)
> > @@ -116,6 +100,11 @@
> >  #define MTK_MAX_SECTOR		(16)
> >  #define MTK_NAND_MAX_NSELS	(2)
> >  
> > +struct mtk_nfc_devdata {
> > +	const u32 *spare_format;
> > +	const u8 *spare_size;
> > +};
> > +
> >  struct mtk_nfc_bad_mark_ctl {
> >  	void (*bm_swap)(struct mtd_info *, u8 *buf, int raw);
> >  	u32 sec;
> > @@ -155,6 +144,7 @@ struct mtk_nfc {
> >  	struct mtk_ecc *ecc;
> >  
> >  	struct device *dev;
> > +	struct mtk_nfc_devdata *devdata;
> >  	void __iomem *regs;
> >  
> >  	struct completion done;
> > @@ -163,6 +153,53 @@ struct mtk_nfc {
> >  	u8 *buffer;
> >  };
> >  
> > +/* NFC Page Format Control Register Spare Size Field Definition */
> > +enum mtk_nfc_spare_format {
> > +	PAGEFMT_SPARE_16,
> > +	PAGEFMT_SPARE_26,
> > +	PAGEFMT_SPARE_27,
> > +	PAGEFMT_SPARE_28,
> > +	PAGEFMT_SPARE_32,
> > +	PAGEFMT_SPARE_36,
> > +	PAGEFMT_SPARE_40,
> > +	PAGEFMT_SPARE_44,
> > +	PAGEFMT_SPARE_48,
> > +	PAGEFMT_SPARE_49,
> > +	PAGEFMT_SPARE_50,
> > +	PAGEFMT_SPARE_51,
> > +	PAGEFMT_SPARE_52,
> > +	PAGEFMT_SPARE_62,
> > +	PAGEFMT_SPARE_63,
> > +	PAGEFMT_SPARE_64,
> > +};
> > +
> > +static const u32 mtk_nfc_spare_format_mt2701[] = {
> > +	[PAGEFMT_SPARE_16]	= (0 << 4),
> > +	[PAGEFMT_SPARE_26]	= (1 << 4),
> > +	[PAGEFMT_SPARE_27]	= (2 << 4),
> > +	[PAGEFMT_SPARE_28]	= (3 << 4),
> > +	[PAGEFMT_SPARE_32]	= (4 << 4),
> > +	[PAGEFMT_SPARE_36]	= (5 << 4),
> > +	[PAGEFMT_SPARE_40]	= (6 << 4),
> > +	[PAGEFMT_SPARE_44]	= (7 << 4),
> > +	[PAGEFMT_SPARE_48]	= (8 << 4),
> > +	[PAGEFMT_SPARE_49]	= (9 << 4),
> > +	[PAGEFMT_SPARE_50]	= (10 << 4),
> > +	[PAGEFMT_SPARE_51]	= (11 << 4),
> > +	[PAGEFMT_SPARE_52]	= (12 << 4),
> > +	[PAGEFMT_SPARE_62]	= (13 << 4),
> > +	[PAGEFMT_SPARE_63]	= (14 << 4),
> > +	[PAGEFMT_SPARE_64]	= (15 << 4),
> > +};
> > +
> > +/*
> > + * supported spare size of each IP
> > + * 255 is used as ending flag
> > + */
> > +static const u8 spare_size_mt2701[] = {
> > +	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
> > +};
> 
> I guess 255 (or 0xff) is a sentinel. That's probably easier to store
> the array size along with the spare_size array in mtk_nfc_devdata.
> 
OK. Will add array size in mtk_nfc_devdata next version.

> > +
> >  static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
> >  {
> >  	return container_of(nand, struct mtk_nfc_nand_chip, nand);
> > @@ -308,6 +345,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> >  	struct nand_chip *chip = mtd_to_nand(mtd);
> >  	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
> >  	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> > +	const u32 *sparefmt = nfc->devdata->spare_format;
> >  	u32 fmt, spare;
> >  
> >  	if (!mtd->writesize)
> > @@ -354,52 +392,52 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> >  
> >  	switch (spare) {
> >  	case 16:
> > -		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_16];
> >  		break;
> >  	case 26:
> > -		fmt |= (PAGEFMT_SPARE_26 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_26];
> >  		break;
> >  	case 27:
> > -		fmt |= (PAGEFMT_SPARE_27 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_27];
> >  		break;
> >  	case 28:
> > -		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_28];
> >  		break;
> >  	case 32:
> > -		fmt |= (PAGEFMT_SPARE_32 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_32];
> >  		break;
> >  	case 36:
> > -		fmt |= (PAGEFMT_SPARE_36 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_36];
> >  		break;
> >  	case 40:
> > -		fmt |= (PAGEFMT_SPARE_40 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_40];
> >  		break;
> >  	case 44:
> > -		fmt |= (PAGEFMT_SPARE_44 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_44];
> >  		break;
> >  	case 48:
> > -		fmt |= (PAGEFMT_SPARE_48 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_48];
> >  		break;
> >  	case 49:
> > -		fmt |= (PAGEFMT_SPARE_49 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_49];
> >  		break;
> >  	case 50:
> > -		fmt |= (PAGEFMT_SPARE_50 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_50];
> >  		break;
> >  	case 51:
> > -		fmt |= (PAGEFMT_SPARE_51 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_51];
> >  		break;
> >  	case 52:
> > -		fmt |= (PAGEFMT_SPARE_52 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_52];
> >  		break;
> >  	case 62:
> > -		fmt |= (PAGEFMT_SPARE_62 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_62];
> >  		break;
> >  	case 63:
> > -		fmt |= (PAGEFMT_SPARE_63 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_63];
> >  		break;
> >  	case 64:
> > -		fmt |= (PAGEFMT_SPARE_64 << PAGEFMT_SPARE_SHIFT);
> > +		fmt |= sparefmt[PAGEFMT_SPARE_64];
> >  		break;
> 
> Could probably be turned into a for loop:
> 
> 	for (i = 0; i < nfc->devdata->num_spare_format; i++) {
> 		if (nfc->devdata->spare_size[i] == spare)
> 			break;
> 	}
> 
> 	if (i == nfc->devdata->num_spare_format) {
> 		dev_err()
> 		return -EINVAL;
> 	}
> 
> 	fmt |= sparefmt[i];
> 

OK.

> >  	default:
> >  		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
> > @@ -408,7 +446,7 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> >  
> >  	fmt |= mtk_nand->fdm.reg_size << PAGEFMT_FDM_SHIFT;
> >  	fmt |= mtk_nand->fdm.ecc_size << PAGEFMT_FDM_ECC_SHIFT;
> > -	nfi_writew(nfc, fmt, NFI_PAGEFMT);
> > +	nfi_writel(nfc, fmt, NFI_PAGEFMT);
> 
> Is this still compatible with the old IP? Maybe this should go in a
> separate patch. 
> 
Yes. NFI_PAGEFMT is always 4Byte length.

I will move this change in a separate patch next version.

> >  
> >  	nfc->ecc_cfg.strength = chip->ecc.strength;
> >  	nfc->ecc_cfg.len = chip->ecc.size + mtk_nand->fdm.ecc_size;
> > @@ -1009,7 +1047,7 @@ static inline void mtk_nfc_hw_init(struct mtk_nfc *nfc)
> >  	 * 0  : poll the status of the busy/ready signal after [7:4]*16 cycles.
> >  	 */
> >  	nfi_writew(nfc, 0xf1, NFI_CNRNB);
> > -	nfi_writew(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
> > +	nfi_writel(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
> >  
> >  	mtk_nfc_hw_reset(nfc);
> >  
> > @@ -1134,8 +1172,8 @@ static void mtk_nfc_set_bad_mark_ctl(struct mtk_nfc_bad_mark_ctl *bm_ctl,
> >  static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
> >  {
> >  	struct nand_chip *nand = mtd_to_nand(mtd);
> > -	u32 spare[] = {16, 26, 27, 28, 32, 36, 40, 44,
> > -			48, 49, 50, 51, 52, 62, 63, 64};
> > +	struct mtk_nfc *nfc = nand_get_controller_data(nand);
> > +	const u8 *spare = nfc->devdata->spare_size;
> >  	u32 eccsteps, i;
> >  
> >  	eccsteps = mtd->writesize / nand->ecc.size;
> > @@ -1144,7 +1182,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
> >  	if (nand->ecc.size == 1024)
> >  		*sps >>= 1;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(spare); i++) {
> > +	for (i = 0; ; i++) {
> 
> With the array size in devdata you could check that i is less than
> num_spare_format.
> BTW, I don't see any check on the sentinel val.
> 
OK.
> >  		if (*sps <= spare[i]) {
> >  			if (!i)
> >  				*sps = spare[i];
> > @@ -1154,9 +1192,6 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
> >  		}
> >  	}
> >  
> > -	if (i >= ARRAY_SIZE(spare))
> > -		*sps = spare[ARRAY_SIZE(spare) - 1];
> > -
> >  	if (nand->ecc.size == 1024)
> >  		*sps <<= 1;
> >  }
> > @@ -1164,6 +1199,7 @@ static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
> >  static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
> >  {
> >  	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct mtk_nfc *nfc = nand_get_controller_data(nand);
> >  	u32 spare;
> >  	int free;
> >  
> > @@ -1214,7 +1250,7 @@ static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
> >  		}
> >  	}
> >  
> > -	mtk_ecc_adjust_strength(&nand->ecc.strength);
> > +	mtk_ecc_adjust_strength(nfc->ecc, &nand->ecc.strength);
> >  
> >  	dev_info(dev, "eccsize %d eccstrength %d\n",
> >  		 nand->ecc.size, nand->ecc.strength);
> > @@ -1354,12 +1390,27 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
> >  	return 0;
> >  }
> >  
> > +static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2701 = {
> > +	.spare_format = mtk_nfc_spare_format_mt2701,
> > +	.spare_size = spare_size_mt2701,
> > +};
> > +
> > +static const struct of_device_id mtk_nfc_id_table[] = {
> > +	{
> > +		.compatible = "mediatek,mt2701-nfc",
> > +		.data = &mtk_nfc_devdata_mt2701,
> > +	},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
> > +
> >  static int mtk_nfc_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> >  	struct mtk_nfc *nfc;
> >  	struct resource *res;
> > +	const struct of_device_id *of_nfc_id = NULL;
> >  	int ret, irq;
> >  
> >  	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> > @@ -1423,6 +1474,13 @@ static int mtk_nfc_probe(struct platform_device *pdev)
> >  		goto clk_disable;
> >  	}
> >  
> > +	of_nfc_id = of_match_device(mtk_nfc_id_table, &pdev->dev);
> > +	if (!of_nfc_id) {
> > +		ret = -ENODEV;
> > +		goto clk_disable;
> > +	}
> 
> Blank line.
> 
OK.

> > +	nfc->devdata = (struct mtk_nfc_devdata *)of_nfc_id->data;
> 
> Cast unneeded.
> 
OK.

> > +
> >  	platform_set_drvdata(pdev, nfc);
> >  
> >  	ret = mtk_nfc_nand_chips_init(dev, nfc);
> > @@ -1503,12 +1561,6 @@ static int mtk_nfc_resume(struct device *dev)
> >  static SIMPLE_DEV_PM_OPS(mtk_nfc_pm_ops, mtk_nfc_suspend, mtk_nfc_resume);
> >  #endif
> >  
> > -static const struct of_device_id mtk_nfc_id_table[] = {
> > -	{ .compatible = "mediatek,mt2701-nfc" },
> > -	{}
> > -};
> > -MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
> > -
> >  static struct platform_driver mtk_nfc_driver = {
> >  	.probe  = mtk_nfc_probe,
> >  	.remove = mtk_nfc_remove,
> 

[PATCH v2 3/3] mtd: nand: mediatek: add support for MT2712 NAND FLASH Controller

[Xiaolei Li]

MT2712 NAND FLASH Controller is similar to MT2701 except those following:
(1) MT2712 supports up to 148B spare size per 1KB size sector (the same
    with 74B spare size per 512B size sector). There are three new spare
    format: 61, 67, 74.
(2) MT2712 supports up to 80 bit ecc strength. There are three new ecc
    strength level: 68, 72, 80.
(3) MT2712 ECC encode parity data register's start offset is 0x300, and
    different with 0x10 of MT2701.
(4) MT2712 improves ecc irq function. When ECC works in ECC_NFI_MODE,
    MT2701 will generate ecc irq number the same with ecc steps during
    page read. However, MT2712 can only generate one ecc irq.

Changes of this patch are:
(1) add two new variables named pg_irq_sel, encode_parity_reg0 in struct
    mtk_ecc_devdata.
(2) add three new ecc level: ECC_CNFG_68BIT, ECC_CNFG_72BIT,
    ECC_CNFG_80BIT.
(3) add new bitfield ECC_PG_IRQ_SEL for register ECC_IRQ_REG.
(4) add three new spare format: PAGEFMT_SPARE_61, PAGEFMT_SPARE_67,
    PAGEFMT_SPARE_74.
(5) add mt2712 nfc and ecc device compatiable and data.

Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
---
 drivers/mtd/nand/mtk_ecc.c  | 45 ++++++++++++++++++++++++++++++++++++----
 drivers/mtd/nand/mtk_nand.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
index 94d0791..ac46fb0 100644
--- a/drivers/mtd/nand/mtk_ecc.c
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -28,6 +28,7 @@
 
 #define ECC_IDLE_MASK		BIT(0)
 #define ECC_IRQ_EN		BIT(0)
+#define ECC_PG_IRQ_SEL		BIT(1)
 #define ECC_OP_ENABLE		(1)
 #define ECC_OP_DISABLE		(0)
 
@@ -53,11 +54,13 @@
 #define		ECC_CNFG_52BIT		(0x11)
 #define		ECC_CNFG_56BIT		(0x12)
 #define		ECC_CNFG_60BIT		(0x13)
+#define		ECC_CNFG_68BIT		(0x14)
+#define		ECC_CNFG_72BIT		(0x15)
+#define		ECC_CNFG_80BIT		(0x16)
 #define		ECC_MODE_SHIFT		(5)
 #define		ECC_MS_SHIFT		(16)
 #define ECC_ENCDIADDR		(0x08)
 #define ECC_ENCIDLE		(0x0C)
-#define ECC_ENCPAR(x)		(0x10 + (x) * sizeof(u32))
 #define ECC_ENCIRQ_EN		(0x80)
 #define ECC_ENCIRQ_STA		(0x84)
 #define ECC_DECCON		(0x100)
@@ -80,6 +83,8 @@
 struct mtk_ecc_devdata {
 	u32 err_mask;
 	u8 max_ecc_strength;
+	u32 encode_parity_reg0;
+	int pg_irq_sel;
 };
 
 struct mtk_ecc {
@@ -207,6 +212,15 @@ static void mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
 	case 60:
 		ecc_bit = ECC_CNFG_60BIT;
 		break;
+	case 68:
+		ecc_bit = ECC_CNFG_68BIT;
+		break;
+	case 72:
+		ecc_bit = ECC_CNFG_72BIT;
+		break;
+	case 80:
+		ecc_bit = ECC_CNFG_80BIT;
+		break;
 	default:
 		dev_err(ecc->dev, "invalid strength %d, default to 4 bits\n",
 			config->strength);
@@ -318,6 +332,7 @@ struct mtk_ecc *of_mtk_ecc_get(struct device_node *of_node)
 int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
 {
 	enum mtk_ecc_operation op = config->op;
+	u16 reg_val;
 	int ret;
 
 	ret = mutex_lock_interruptible(&ecc->lock);
@@ -331,7 +346,15 @@ int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
 	writew(ECC_OP_ENABLE, ecc->regs + ECC_CTL_REG(op));
 
 	init_completion(&ecc->done);
-	writew(ECC_IRQ_EN, ecc->regs + ECC_IRQ_REG(op));
+	reg_val = ECC_IRQ_EN;
+	/*
+	 * For ECC_NFI_MODE, if ecc->devdata->pg_irq_sel is 1, then it
+	 * means this chip can only generate one ecc irq during page
+	 * read / write. If is 0, generate one ecc irq each ecc step.
+	 */
+	if ((ecc->devdata->pg_irq_sel) && (config->mode == ECC_NFI_MODE))
+		reg_val |= ECC_PG_IRQ_SEL;
+	writew(reg_val, ecc->regs + ECC_IRQ_REG(op));
 
 	return 0;
 }
@@ -401,7 +424,9 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
 	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
 
 	/* write the parity bytes generated by the ECC back to temp buffer */
-	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
+	__ioread32_copy(ecc->eccdata,
+			ecc->regs + ecc->devdata->encode_parity_reg0,
+			round_up(len, 4));
 
 	/* copy into possibly unaligned OOB region with actual length */
 	memcpy(data + bytes, ecc->eccdata, len);
@@ -417,7 +442,7 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
 void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
 {
 	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
-				40, 44, 48, 52, 56, 60};
+				40, 44, 48, 52, 56, 60, 68, 72, 80};
 	int i;
 
 	if (*p >= ecc->devdata->max_ecc_strength) {
@@ -442,12 +467,24 @@ void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
 static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
 	.err_mask = 0x3f,
 	.max_ecc_strength = 60,
+	.encode_parity_reg0 = 0x10,
+	.pg_irq_sel = 0,
+};
+
+static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2712 = {
+	.err_mask = 0x7f,
+	.max_ecc_strength = 80,
+	.encode_parity_reg0 = 0x300,
+	.pg_irq_sel = 1,
 };
 
 static const struct of_device_id mtk_ecc_dt_match[] = {
 	{
 		.compatible = "mediatek,mt2701-ecc",
 		.data = &mtk_ecc_devdata_mt2701,
+	}, {
+		.compatible = "mediatek,mt2712-ecc",
+		.data = &mtk_ecc_devdata_mt2712,
 	},
 	{},
 };
diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
index 1a64ea2..5bdeace 100644
--- a/drivers/mtd/nand/mtk_nand.c
+++ b/drivers/mtd/nand/mtk_nand.c
@@ -168,9 +168,12 @@ enum mtk_nfc_spare_format {
 	PAGEFMT_SPARE_50,
 	PAGEFMT_SPARE_51,
 	PAGEFMT_SPARE_52,
+	PAGEFMT_SPARE_61,
 	PAGEFMT_SPARE_62,
 	PAGEFMT_SPARE_63,
 	PAGEFMT_SPARE_64,
+	PAGEFMT_SPARE_67,
+	PAGEFMT_SPARE_74,
 };
 
 static const u32 mtk_nfc_spare_format_mt2701[] = {
@@ -187,9 +190,34 @@ enum mtk_nfc_spare_format {
 	[PAGEFMT_SPARE_50]	= (10 << 4),
 	[PAGEFMT_SPARE_51]	= (11 << 4),
 	[PAGEFMT_SPARE_52]	= (12 << 4),
+	[PAGEFMT_SPARE_61]	= (0 << 4),
 	[PAGEFMT_SPARE_62]	= (13 << 4),
 	[PAGEFMT_SPARE_63]	= (14 << 4),
 	[PAGEFMT_SPARE_64]	= (15 << 4),
+	[PAGEFMT_SPARE_67]	= (0 << 4),
+	[PAGEFMT_SPARE_74]	= (0 << 4),
+};
+
+static const u32 mtk_nfc_spare_format_mt2712[] = {
+	[PAGEFMT_SPARE_16]	= (0 << 16),
+	[PAGEFMT_SPARE_26]	= (1 << 16),
+	[PAGEFMT_SPARE_27]	= (2 << 16),
+	[PAGEFMT_SPARE_28]	= (3 << 16),
+	[PAGEFMT_SPARE_32]	= (4 << 16),
+	[PAGEFMT_SPARE_36]	= (5 << 16),
+	[PAGEFMT_SPARE_40]	= (6 << 16),
+	[PAGEFMT_SPARE_44]	= (7 << 16),
+	[PAGEFMT_SPARE_48]	= (8 << 16),
+	[PAGEFMT_SPARE_49]	= (9 << 16),
+	[PAGEFMT_SPARE_50]	= (10 << 16),
+	[PAGEFMT_SPARE_51]	= (11 << 16),
+	[PAGEFMT_SPARE_52]	= (12 << 16),
+	[PAGEFMT_SPARE_61]	= (14 << 16),
+	[PAGEFMT_SPARE_62]	= (13 << 16),
+	[PAGEFMT_SPARE_63]	= (15 << 16),
+	[PAGEFMT_SPARE_64]	= (16 << 16),
+	[PAGEFMT_SPARE_67]	= (17 << 16),
+	[PAGEFMT_SPARE_74]	= (18 << 16),
 };
 
 /*
@@ -200,6 +228,11 @@ enum mtk_nfc_spare_format {
 	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
 };
 
+static const u8 spare_size_mt2712[] = {
+	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 61, 62, 63, 64, 67,
+	74, 255
+};
+
 static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
 {
 	return container_of(nand, struct mtk_nfc_nand_chip, nand);
@@ -430,6 +463,9 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
 	case 52:
 		fmt |= sparefmt[PAGEFMT_SPARE_52];
 		break;
+	case 61:
+		fmt |= sparefmt[PAGEFMT_SPARE_61];
+		break;
 	case 62:
 		fmt |= sparefmt[PAGEFMT_SPARE_62];
 		break;
@@ -439,6 +475,12 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
 	case 64:
 		fmt |= sparefmt[PAGEFMT_SPARE_64];
 		break;
+	case 67:
+		fmt |= sparefmt[PAGEFMT_SPARE_67];
+		break;
+	case 74:
+		fmt |= sparefmt[PAGEFMT_SPARE_74];
+		break;
 	default:
 		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
 		return -EINVAL;
@@ -1395,10 +1437,18 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
 	.spare_size = spare_size_mt2701,
 };
 
+static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2712 = {
+	.spare_format = mtk_nfc_spare_format_mt2712,
+	.spare_size = spare_size_mt2712,
+};
+
 static const struct of_device_id mtk_nfc_id_table[] = {
 	{
 		.compatible = "mediatek,mt2701-nfc",
 		.data = &mtk_nfc_devdata_mt2701,
+	}, {
+		.compatible = "mediatek,mt2712-nfc",
+		.data = &mtk_nfc_devdata_mt2712,
 	},
 	{}
 };
-- 
1.9.1

Re: [PATCH v2 3/3] mtd: nand: mediatek: add support for MT2712 NAND FLASH Controller

[Boris Brezillon]

On Fri, 12 May 2017 12:33:23 +0800
Xiaolei Li <xiaolei.li@mediatek.com> wrote:

> MT2712 NAND FLASH Controller is similar to MT2701 except those following:
> (1) MT2712 supports up to 148B spare size per 1KB size sector (the same
>     with 74B spare size per 512B size sector). There are three new spare
>     format: 61, 67, 74.
> (2) MT2712 supports up to 80 bit ecc strength. There are three new ecc
>     strength level: 68, 72, 80.
> (3) MT2712 ECC encode parity data register's start offset is 0x300, and
>     different with 0x10 of MT2701.
> (4) MT2712 improves ecc irq function. When ECC works in ECC_NFI_MODE,
>     MT2701 will generate ecc irq number the same with ecc steps during
>     page read. However, MT2712 can only generate one ecc irq.
> 
> Changes of this patch are:
> (1) add two new variables named pg_irq_sel, encode_parity_reg0 in struct
>     mtk_ecc_devdata.
> (2) add three new ecc level: ECC_CNFG_68BIT, ECC_CNFG_72BIT,
>     ECC_CNFG_80BIT.
> (3) add new bitfield ECC_PG_IRQ_SEL for register ECC_IRQ_REG.
> (4) add three new spare format: PAGEFMT_SPARE_61, PAGEFMT_SPARE_67,
>     PAGEFMT_SPARE_74.
> (5) add mt2712 nfc and ecc device compatiable and data.
> 
> Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
> ---
>  drivers/mtd/nand/mtk_ecc.c  | 45 ++++++++++++++++++++++++++++++++++++----
>  drivers/mtd/nand/mtk_nand.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> index 94d0791..ac46fb0 100644
> --- a/drivers/mtd/nand/mtk_ecc.c
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -28,6 +28,7 @@
>  
>  #define ECC_IDLE_MASK		BIT(0)
>  #define ECC_IRQ_EN		BIT(0)
> +#define ECC_PG_IRQ_SEL		BIT(1)
>  #define ECC_OP_ENABLE		(1)
>  #define ECC_OP_DISABLE		(0)
>  
> @@ -53,11 +54,13 @@
>  #define		ECC_CNFG_52BIT		(0x11)
>  #define		ECC_CNFG_56BIT		(0x12)
>  #define		ECC_CNFG_60BIT		(0x13)
> +#define		ECC_CNFG_68BIT		(0x14)
> +#define		ECC_CNFG_72BIT		(0x15)
> +#define		ECC_CNFG_80BIT		(0x16)

We could get rid of these defs and just use the index of the strength
table (named ecc_level in your patch).

>  #define		ECC_MODE_SHIFT		(5)
>  #define		ECC_MS_SHIFT		(16)
>  #define ECC_ENCDIADDR		(0x08)
>  #define ECC_ENCIDLE		(0x0C)
> -#define ECC_ENCPAR(x)		(0x10 + (x) * sizeof(u32))
>  #define ECC_ENCIRQ_EN		(0x80)
>  #define ECC_ENCIRQ_STA		(0x84)
>  #define ECC_DECCON		(0x100)
> @@ -80,6 +83,8 @@
>  struct mtk_ecc_devdata {
>  	u32 err_mask;
>  	u8 max_ecc_strength;
> +	u32 encode_parity_reg0;
> +	int pg_irq_sel;
>  };
>  
>  struct mtk_ecc {
> @@ -207,6 +212,15 @@ static void mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
>  	case 60:
>  		ecc_bit = ECC_CNFG_60BIT;
>  		break;
> +	case 68:
> +		ecc_bit = ECC_CNFG_68BIT;
> +		break;
> +	case 72:
> +		ecc_bit = ECC_CNFG_72BIT;
> +		break;
> +	case 80:
> +		ecc_bit = ECC_CNFG_80BIT;
> +		break;
>  	default:
>  		dev_err(ecc->dev, "invalid strength %d, default to 4 bits\n",
>  			config->strength);
> @@ -318,6 +332,7 @@ struct mtk_ecc *of_mtk_ecc_get(struct device_node *of_node)
>  int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
>  {
>  	enum mtk_ecc_operation op = config->op;
> +	u16 reg_val;
>  	int ret;
>  
>  	ret = mutex_lock_interruptible(&ecc->lock);
> @@ -331,7 +346,15 @@ int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
>  	writew(ECC_OP_ENABLE, ecc->regs + ECC_CTL_REG(op));
>  
>  	init_completion(&ecc->done);
> -	writew(ECC_IRQ_EN, ecc->regs + ECC_IRQ_REG(op));
> +	reg_val = ECC_IRQ_EN;
> +	/*
> +	 * For ECC_NFI_MODE, if ecc->devdata->pg_irq_sel is 1, then it
> +	 * means this chip can only generate one ecc irq during page
> +	 * read / write. If is 0, generate one ecc irq each ecc step.
> +	 */
> +	if ((ecc->devdata->pg_irq_sel) && (config->mode == ECC_NFI_MODE))
> +		reg_val |= ECC_PG_IRQ_SEL;
> +	writew(reg_val, ecc->regs + ECC_IRQ_REG(op));
>  
>  	return 0;
>  }
> @@ -401,7 +424,9 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>  	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
>  
>  	/* write the parity bytes generated by the ECC back to temp buffer */
> -	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> +	__ioread32_copy(ecc->eccdata,
> +			ecc->regs + ecc->devdata->encode_parity_reg0,
> +			round_up(len, 4));
>  
>  	/* copy into possibly unaligned OOB region with actual length */
>  	memcpy(data + bytes, ecc->eccdata, len);
> @@ -417,7 +442,7 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>  void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
>  {
>  	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> -				40, 44, 48, 52, 56, 60};
> +				40, 44, 48, 52, 56, 60, 68, 72, 80};

Please try to align on the open curly brace.

>  	int i;
>  
>  	if (*p >= ecc->devdata->max_ecc_strength) {
> @@ -442,12 +467,24 @@ void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
>  static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
>  	.err_mask = 0x3f,
>  	.max_ecc_strength = 60,
> +	.encode_parity_reg0 = 0x10,
> +	.pg_irq_sel = 0,
> +};
> +
> +static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2712 = {
> +	.err_mask = 0x7f,
> +	.max_ecc_strength = 80,
> +	.encode_parity_reg0 = 0x300,
> +	.pg_irq_sel = 1,
>  };
>  
>  static const struct of_device_id mtk_ecc_dt_match[] = {
>  	{
>  		.compatible = "mediatek,mt2701-ecc",
>  		.data = &mtk_ecc_devdata_mt2701,
> +	}, {
> +		.compatible = "mediatek,mt2712-ecc",
> +		.data = &mtk_ecc_devdata_mt2712,
>  	},
>  	{},
>  };
> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> index 1a64ea2..5bdeace 100644
> --- a/drivers/mtd/nand/mtk_nand.c
> +++ b/drivers/mtd/nand/mtk_nand.c
> @@ -168,9 +168,12 @@ enum mtk_nfc_spare_format {
>  	PAGEFMT_SPARE_50,
>  	PAGEFMT_SPARE_51,
>  	PAGEFMT_SPARE_52,
> +	PAGEFMT_SPARE_61,
>  	PAGEFMT_SPARE_62,
>  	PAGEFMT_SPARE_63,
>  	PAGEFMT_SPARE_64,
> +	PAGEFMT_SPARE_67,
> +	PAGEFMT_SPARE_74,
>  };
>  
>  static const u32 mtk_nfc_spare_format_mt2701[] = {
> @@ -187,9 +190,34 @@ enum mtk_nfc_spare_format {
>  	[PAGEFMT_SPARE_50]	= (10 << 4),
>  	[PAGEFMT_SPARE_51]	= (11 << 4),
>  	[PAGEFMT_SPARE_52]	= (12 << 4),
> +	[PAGEFMT_SPARE_61]	= (0 << 4),
>  	[PAGEFMT_SPARE_62]	= (13 << 4),
>  	[PAGEFMT_SPARE_63]	= (14 << 4),
>  	[PAGEFMT_SPARE_64]	= (15 << 4),
> +	[PAGEFMT_SPARE_67]	= (0 << 4),
> +	[PAGEFMT_SPARE_74]	= (0 << 4),
> +};
> +
> +static const u32 mtk_nfc_spare_format_mt2712[] = {
> +	[PAGEFMT_SPARE_16]	= (0 << 16),
> +	[PAGEFMT_SPARE_26]	= (1 << 16),
> +	[PAGEFMT_SPARE_27]	= (2 << 16),
> +	[PAGEFMT_SPARE_28]	= (3 << 16),
> +	[PAGEFMT_SPARE_32]	= (4 << 16),
> +	[PAGEFMT_SPARE_36]	= (5 << 16),
> +	[PAGEFMT_SPARE_40]	= (6 << 16),
> +	[PAGEFMT_SPARE_44]	= (7 << 16),
> +	[PAGEFMT_SPARE_48]	= (8 << 16),
> +	[PAGEFMT_SPARE_49]	= (9 << 16),
> +	[PAGEFMT_SPARE_50]	= (10 << 16),
> +	[PAGEFMT_SPARE_51]	= (11 << 16),
> +	[PAGEFMT_SPARE_52]	= (12 << 16),
> +	[PAGEFMT_SPARE_61]	= (14 << 16),
> +	[PAGEFMT_SPARE_62]	= (13 << 16),
> +	[PAGEFMT_SPARE_63]	= (15 << 16),
> +	[PAGEFMT_SPARE_64]	= (16 << 16),
> +	[PAGEFMT_SPARE_67]	= (17 << 16),
> +	[PAGEFMT_SPARE_74]	= (18 << 16),
>  };

It really looks like the page format id is the index of the spare_size
array. I think you can get rid of the PAGEFMT_SPARE_ definitions if you
use the for loop I suggested in patch 2.

>  
>  /*
> @@ -200,6 +228,11 @@ enum mtk_nfc_spare_format {
>  	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
>  };
>  
> +static const u8 spare_size_mt2712[] = {
> +	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 61, 62, 63, 64, 67,
> +	74, 255
> +};
> +
>  static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
>  {
>  	return container_of(nand, struct mtk_nfc_nand_chip, nand);
> @@ -430,6 +463,9 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
>  	case 52:
>  		fmt |= sparefmt[PAGEFMT_SPARE_52];
>  		break;
> +	case 61:
> +		fmt |= sparefmt[PAGEFMT_SPARE_61];
> +		break;
>  	case 62:
>  		fmt |= sparefmt[PAGEFMT_SPARE_62];
>  		break;
> @@ -439,6 +475,12 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
>  	case 64:
>  		fmt |= sparefmt[PAGEFMT_SPARE_64];
>  		break;
> +	case 67:
> +		fmt |= sparefmt[PAGEFMT_SPARE_67];
> +		break;
> +	case 74:
> +		fmt |= sparefmt[PAGEFMT_SPARE_74];
> +		break;
>  	default:
>  		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
>  		return -EINVAL;
> @@ -1395,10 +1437,18 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
>  	.spare_size = spare_size_mt2701,
>  };
>  
> +static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2712 = {
> +	.spare_format = mtk_nfc_spare_format_mt2712,
> +	.spare_size = spare_size_mt2712,
> +};
> +
>  static const struct of_device_id mtk_nfc_id_table[] = {
>  	{
>  		.compatible = "mediatek,mt2701-nfc",
>  		.data = &mtk_nfc_devdata_mt2701,
> +	}, {
> +		.compatible = "mediatek,mt2712-nfc",
> +		.data = &mtk_nfc_devdata_mt2712,
>  	},
>  	{}
>  };

Re: [PATCH v2 3/3] mtd: nand: mediatek: add support for MT2712 NAND FLASH Controller

[xiaolei li]

On Fri, 2017-05-12 at 08:59 +0200, Boris Brezillon wrote:
> On Fri, 12 May 2017 12:33:23 +0800
> Xiaolei Li <xiaolei.li@mediatek.com> wrote:
> 
> > MT2712 NAND FLASH Controller is similar to MT2701 except those following:
> > (1) MT2712 supports up to 148B spare size per 1KB size sector (the same
> >     with 74B spare size per 512B size sector). There are three new spare
> >     format: 61, 67, 74.
> > (2) MT2712 supports up to 80 bit ecc strength. There are three new ecc
> >     strength level: 68, 72, 80.
> > (3) MT2712 ECC encode parity data register's start offset is 0x300, and
> >     different with 0x10 of MT2701.
> > (4) MT2712 improves ecc irq function. When ECC works in ECC_NFI_MODE,
> >     MT2701 will generate ecc irq number the same with ecc steps during
> >     page read. However, MT2712 can only generate one ecc irq.
> > 
> > Changes of this patch are:
> > (1) add two new variables named pg_irq_sel, encode_parity_reg0 in struct
> >     mtk_ecc_devdata.
> > (2) add three new ecc level: ECC_CNFG_68BIT, ECC_CNFG_72BIT,
> >     ECC_CNFG_80BIT.
> > (3) add new bitfield ECC_PG_IRQ_SEL for register ECC_IRQ_REG.
> > (4) add three new spare format: PAGEFMT_SPARE_61, PAGEFMT_SPARE_67,
> >     PAGEFMT_SPARE_74.
> > (5) add mt2712 nfc and ecc device compatiable and data.
> > 
> > Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
> > ---
> >  drivers/mtd/nand/mtk_ecc.c  | 45 ++++++++++++++++++++++++++++++++++++----
> >  drivers/mtd/nand/mtk_nand.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 91 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> > index 94d0791..ac46fb0 100644
> > --- a/drivers/mtd/nand/mtk_ecc.c
> > +++ b/drivers/mtd/nand/mtk_ecc.c
> > @@ -28,6 +28,7 @@
> >  
> >  #define ECC_IDLE_MASK		BIT(0)
> >  #define ECC_IRQ_EN		BIT(0)
> > +#define ECC_PG_IRQ_SEL		BIT(1)
> >  #define ECC_OP_ENABLE		(1)
> >  #define ECC_OP_DISABLE		(0)
> >  
> > @@ -53,11 +54,13 @@
> >  #define		ECC_CNFG_52BIT		(0x11)
> >  #define		ECC_CNFG_56BIT		(0x12)
> >  #define		ECC_CNFG_60BIT		(0x13)
> > +#define		ECC_CNFG_68BIT		(0x14)
> > +#define		ECC_CNFG_72BIT		(0x15)
> > +#define		ECC_CNFG_80BIT		(0x16)
> 
> We could get rid of these defs and just use the index of the strength
> table (named ecc_level in your patch).

OK.

> 
> >  #define		ECC_MODE_SHIFT		(5)
> >  #define		ECC_MS_SHIFT		(16)
> >  #define ECC_ENCDIADDR		(0x08)
> >  #define ECC_ENCIDLE		(0x0C)
> > -#define ECC_ENCPAR(x)		(0x10 + (x) * sizeof(u32))
> >  #define ECC_ENCIRQ_EN		(0x80)
> >  #define ECC_ENCIRQ_STA		(0x84)
> >  #define ECC_DECCON		(0x100)
> > @@ -80,6 +83,8 @@
> >  struct mtk_ecc_devdata {
> >  	u32 err_mask;
> >  	u8 max_ecc_strength;
> > +	u32 encode_parity_reg0;
> > +	int pg_irq_sel;
> >  };
> >  
> >  struct mtk_ecc {
> > @@ -207,6 +212,15 @@ static void mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
> >  	case 60:
> >  		ecc_bit = ECC_CNFG_60BIT;
> >  		break;
> > +	case 68:
> > +		ecc_bit = ECC_CNFG_68BIT;
> > +		break;
> > +	case 72:
> > +		ecc_bit = ECC_CNFG_72BIT;
> > +		break;
> > +	case 80:
> > +		ecc_bit = ECC_CNFG_80BIT;
> > +		break;
> >  	default:
> >  		dev_err(ecc->dev, "invalid strength %d, default to 4 bits\n",
> >  			config->strength);
> > @@ -318,6 +332,7 @@ struct mtk_ecc *of_mtk_ecc_get(struct device_node *of_node)
> >  int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
> >  {
> >  	enum mtk_ecc_operation op = config->op;
> > +	u16 reg_val;
> >  	int ret;
> >  
> >  	ret = mutex_lock_interruptible(&ecc->lock);
> > @@ -331,7 +346,15 @@ int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
> >  	writew(ECC_OP_ENABLE, ecc->regs + ECC_CTL_REG(op));
> >  
> >  	init_completion(&ecc->done);
> > -	writew(ECC_IRQ_EN, ecc->regs + ECC_IRQ_REG(op));
> > +	reg_val = ECC_IRQ_EN;
> > +	/*
> > +	 * For ECC_NFI_MODE, if ecc->devdata->pg_irq_sel is 1, then it
> > +	 * means this chip can only generate one ecc irq during page
> > +	 * read / write. If is 0, generate one ecc irq each ecc step.
> > +	 */
> > +	if ((ecc->devdata->pg_irq_sel) && (config->mode == ECC_NFI_MODE))
> > +		reg_val |= ECC_PG_IRQ_SEL;
> > +	writew(reg_val, ecc->regs + ECC_IRQ_REG(op));
> >  
> >  	return 0;
> >  }
> > @@ -401,7 +424,9 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> >  	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
> >  
> >  	/* write the parity bytes generated by the ECC back to temp buffer */
> > -	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> > +	__ioread32_copy(ecc->eccdata,
> > +			ecc->regs + ecc->devdata->encode_parity_reg0,
> > +			round_up(len, 4));
> >  
> >  	/* copy into possibly unaligned OOB region with actual length */
> >  	memcpy(data + bytes, ecc->eccdata, len);
> > @@ -417,7 +442,7 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> >  void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
> >  {
> >  	u32 ecc_level[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> > -				40, 44, 48, 52, 56, 60};
> > +				40, 44, 48, 52, 56, 60, 68, 72, 80};
> 
> Please try to align on the open curly brace.
> 
OK.

> >  	int i;
> >  
> >  	if (*p >= ecc->devdata->max_ecc_strength) {
> > @@ -442,12 +467,24 @@ void mtk_ecc_adjust_strength(struct mtk_ecc *ecc, u32 *p)
> >  static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2701 = {
> >  	.err_mask = 0x3f,
> >  	.max_ecc_strength = 60,
> > +	.encode_parity_reg0 = 0x10,
> > +	.pg_irq_sel = 0,
> > +};
> > +
> > +static const struct mtk_ecc_devdata mtk_ecc_devdata_mt2712 = {
> > +	.err_mask = 0x7f,
> > +	.max_ecc_strength = 80,
> > +	.encode_parity_reg0 = 0x300,
> > +	.pg_irq_sel = 1,
> >  };
> >  
> >  static const struct of_device_id mtk_ecc_dt_match[] = {
> >  	{
> >  		.compatible = "mediatek,mt2701-ecc",
> >  		.data = &mtk_ecc_devdata_mt2701,
> > +	}, {
> > +		.compatible = "mediatek,mt2712-ecc",
> > +		.data = &mtk_ecc_devdata_mt2712,
> >  	},
> >  	{},
> >  };
> > diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> > index 1a64ea2..5bdeace 100644
> > --- a/drivers/mtd/nand/mtk_nand.c
> > +++ b/drivers/mtd/nand/mtk_nand.c
> > @@ -168,9 +168,12 @@ enum mtk_nfc_spare_format {
> >  	PAGEFMT_SPARE_50,
> >  	PAGEFMT_SPARE_51,
> >  	PAGEFMT_SPARE_52,
> > +	PAGEFMT_SPARE_61,
> >  	PAGEFMT_SPARE_62,
> >  	PAGEFMT_SPARE_63,
> >  	PAGEFMT_SPARE_64,
> > +	PAGEFMT_SPARE_67,
> > +	PAGEFMT_SPARE_74,
> >  };
> >  
> >  static const u32 mtk_nfc_spare_format_mt2701[] = {
> > @@ -187,9 +190,34 @@ enum mtk_nfc_spare_format {
> >  	[PAGEFMT_SPARE_50]	= (10 << 4),
> >  	[PAGEFMT_SPARE_51]	= (11 << 4),
> >  	[PAGEFMT_SPARE_52]	= (12 << 4),
> > +	[PAGEFMT_SPARE_61]	= (0 << 4),
> >  	[PAGEFMT_SPARE_62]	= (13 << 4),
> >  	[PAGEFMT_SPARE_63]	= (14 << 4),
> >  	[PAGEFMT_SPARE_64]	= (15 << 4),
> > +	[PAGEFMT_SPARE_67]	= (0 << 4),
> > +	[PAGEFMT_SPARE_74]	= (0 << 4),
> > +};
> > +
> > +static const u32 mtk_nfc_spare_format_mt2712[] = {
> > +	[PAGEFMT_SPARE_16]	= (0 << 16),
> > +	[PAGEFMT_SPARE_26]	= (1 << 16),
> > +	[PAGEFMT_SPARE_27]	= (2 << 16),
> > +	[PAGEFMT_SPARE_28]	= (3 << 16),
> > +	[PAGEFMT_SPARE_32]	= (4 << 16),
> > +	[PAGEFMT_SPARE_36]	= (5 << 16),
> > +	[PAGEFMT_SPARE_40]	= (6 << 16),
> > +	[PAGEFMT_SPARE_44]	= (7 << 16),
> > +	[PAGEFMT_SPARE_48]	= (8 << 16),
> > +	[PAGEFMT_SPARE_49]	= (9 << 16),
> > +	[PAGEFMT_SPARE_50]	= (10 << 16),
> > +	[PAGEFMT_SPARE_51]	= (11 << 16),
> > +	[PAGEFMT_SPARE_52]	= (12 << 16),
> > +	[PAGEFMT_SPARE_61]	= (14 << 16),
> > +	[PAGEFMT_SPARE_62]	= (13 << 16),
> > +	[PAGEFMT_SPARE_63]	= (15 << 16),
> > +	[PAGEFMT_SPARE_64]	= (16 << 16),
> > +	[PAGEFMT_SPARE_67]	= (17 << 16),
> > +	[PAGEFMT_SPARE_74]	= (18 << 16),
> >  };
> 
> It really looks like the page format id is the index of the spare_size
> array. I think you can get rid of the PAGEFMT_SPARE_ definitions if you
> use the for loop I suggested in patch 2.
> 
OK. follow your suggestion.

> >  
> >  /*
> > @@ -200,6 +228,11 @@ enum mtk_nfc_spare_format {
> >  	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 62, 63, 64, 255
> >  };
> >  
> > +static const u8 spare_size_mt2712[] = {
> > +	16, 26, 27, 28, 32, 36, 40, 44,	48, 49, 50, 51, 52, 61, 62, 63, 64, 67,
> > +	74, 255
> > +};
> > +
> >  static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
> >  {
> >  	return container_of(nand, struct mtk_nfc_nand_chip, nand);
> > @@ -430,6 +463,9 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> >  	case 52:
> >  		fmt |= sparefmt[PAGEFMT_SPARE_52];
> >  		break;
> > +	case 61:
> > +		fmt |= sparefmt[PAGEFMT_SPARE_61];
> > +		break;
> >  	case 62:
> >  		fmt |= sparefmt[PAGEFMT_SPARE_62];
> >  		break;
> > @@ -439,6 +475,12 @@ static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> >  	case 64:
> >  		fmt |= sparefmt[PAGEFMT_SPARE_64];
> >  		break;
> > +	case 67:
> > +		fmt |= sparefmt[PAGEFMT_SPARE_67];
> > +		break;
> > +	case 74:
> > +		fmt |= sparefmt[PAGEFMT_SPARE_74];
> > +		break;
> >  	default:
> >  		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
> >  		return -EINVAL;
> > @@ -1395,10 +1437,18 @@ static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
> >  	.spare_size = spare_size_mt2701,
> >  };
> >  
> > +static const struct mtk_nfc_devdata mtk_nfc_devdata_mt2712 = {
> > +	.spare_format = mtk_nfc_spare_format_mt2712,
> > +	.spare_size = spare_size_mt2712,
> > +};
> > +
> >  static const struct of_device_id mtk_nfc_id_table[] = {
> >  	{
> >  		.compatible = "mediatek,mt2701-nfc",
> >  		.data = &mtk_nfc_devdata_mt2701,
> > +	}, {
> > +		.compatible = "mediatek,mt2712-nfc",
> > +		.data = &mtk_nfc_devdata_mt2712,
> >  	},
> >  	{}
> >  };
>