[PATCH v1 0/5] mtd: atmel_nand: Add support for NAND Flash on SAMA5D2

[PATCH v1 0/5] mtd: atmel_nand: Add support for NAND Flash on SAMA5D2

[Romain Izard]

The NAND Flash controller for SAMA5D2 has small differences that make it
impossible to use the existing driver for SAMA5D3. This patchset uses the
device tree's compatible string to get those differences, and the code is
slightly modified to handle them.

Applies on the Atmel Linux 4.1 branch for now, but only the dtsi file
differs from the mainline.

Compile tested for all SAMA5 chips for now, and tested on SAMA5D3xek as
there is no existing SAMA5D2 board with NAND Flash memory (yet).

RFC -> V1:
- Split the support of 32-bit ECC on SAMA5D2 in two commits
- Fix the missing ECC strength configuration code

Romain Izard (5):
  mtd: atmel_nand: Do not warn on bitflips
  mtd: atmel_nand: Support variable RB_EDGE interrupts
  mtd: atmel_nand: Support PMECC on SAMA5D2
  mtd: atmel_nand: Support 32-bit ECC strength
  ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

 .../devicetree/bindings/mtd/atmel-nand.txt         | 12 ++-
 arch/arm/boot/dts/sama5d2.dtsi                     | 38 +++++++++
 drivers/mtd/nand/atmel_nand.c                      | 89 ++++++++++++++++++----
 drivers/mtd/nand/atmel_nand_ecc.h                  |  9 ++-
 drivers/mtd/nand/atmel_nand_nfc.h                  |  5 +-
 5 files changed, 131 insertions(+), 22 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 0/5] mtd: atmel_nand: Add support for NAND Flash on SAMA5D2

[Romain Izard]

The NAND Flash controller for SAMA5D2 has small differences that make it
impossible to use the existing driver for SAMA5D3. This patchset uses the
device tree's compatible string to get those differences, and the code is
slightly modified to handle them.

Applies on the Atmel Linux 4.1 branch for now, but only the dtsi file
differs from the mainline.

Compile tested for all SAMA5 chips for now, and tested on SAMA5D3xek as
there is no existing SAMA5D2 board with NAND Flash memory (yet).

RFC -> V1:
- Split the support of 32-bit ECC on SAMA5D2 in two commits
- Fix the missing ECC strength configuration code

Romain Izard (5):
  mtd: atmel_nand: Do not warn on bitflips
  mtd: atmel_nand: Support variable RB_EDGE interrupts
  mtd: atmel_nand: Support PMECC on SAMA5D2
  mtd: atmel_nand: Support 32-bit ECC strength
  ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

 .../devicetree/bindings/mtd/atmel-nand.txt         | 12 ++-
 arch/arm/boot/dts/sama5d2.dtsi                     | 38 +++++++++
 drivers/mtd/nand/atmel_nand.c                      | 89 ++++++++++++++++++----
 drivers/mtd/nand/atmel_nand_ecc.h                  |  9 ++-
 drivers/mtd/nand/atmel_nand_nfc.h                  |  5 +-
 5 files changed, 131 insertions(+), 22 deletions(-)

-- 
2.5.0

[PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Romain Izard]

When using multi-bit ECC, it is normal for the NAND Flash driver to
correct bit errors during the life of the product. Those errors will
only be cleared once a threshold has been reached, and corrections can
occur regularly before this.

Use only dev_dbg and not dev_info to report the bitflips, to keep the
system log clean when everything works correctly.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by:  Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/nand/atmel_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 46010bd895b1..9d71f9e6a8de 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -824,7 +824,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 			*(buf + byte_pos) ^= (1 << bit_pos);
 
 			pos = sector_num * host->pmecc_sector_size + byte_pos;
-			dev_info(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
+			dev_dbg(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
 				pos, bit_pos, err_byte, *(buf + byte_pos));
 		} else {
 			/* Bit flip in OOB area */
@@ -834,7 +834,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 			ecc[tmp] ^= (1 << bit_pos);
 
 			pos = tmp + nand_chip->ecc.layout->eccpos[0];
-			dev_info(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
+			dev_dbg(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
 				pos, bit_pos, err_byte, ecc[tmp]);
 		}
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Romain Izard]

When using multi-bit ECC, it is normal for the NAND Flash driver to
correct bit errors during the life of the product. Those errors will
only be cleared once a threshold has been reached, and corrections can
occur regularly before this.

Use only dev_dbg and not dev_info to report the bitflips, to keep the
system log clean when everything works correctly.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
Acked-by:  Wenyou Yang <wenyou.yang@atmel.com>
---
 drivers/mtd/nand/atmel_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 46010bd895b1..9d71f9e6a8de 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -824,7 +824,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 			*(buf + byte_pos) ^= (1 << bit_pos);
 
 			pos = sector_num * host->pmecc_sector_size + byte_pos;
-			dev_info(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
+			dev_dbg(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
 				pos, bit_pos, err_byte, *(buf + byte_pos));
 		} else {
 			/* Bit flip in OOB area */
@@ -834,7 +834,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 			ecc[tmp] ^= (1 << bit_pos);
 
 			pos = tmp + nand_chip->ecc.layout->eccpos[0];
-			dev_info(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
+			dev_dbg(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
 				pos, bit_pos, err_byte, ecc[tmp]);
 		}
 
-- 
2.5.0

[PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Romain Izard]

The NFC controller used to accelerate the NAND transfers on SAMA5 chips
can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.

Use the controller's compatible string to select the correct bit.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Wenyou Yang <Wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
 drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
 drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 7d4c8eb775a5..89b0db9801b0 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -34,7 +34,7 @@ Optional properties:
 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
 - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
   - Required properties:
-    - compatible : "atmel,sama5d3-nfc".
+    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
     - reg : should specify the address and size used for NFC command registers,
             NFC registers and NFC Sram. NFC Sram address and size can be absent
             if don't want to use it.
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 9d71f9e6a8de..e5d7e7e63f49 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -67,6 +67,10 @@ struct atmel_nand_caps {
 	bool pmecc_correct_erase_page;
 };
 
+struct atmel_nand_nfc_priv {
+	uint32_t rb_edge;
+};
+
 /* oob layout for large page size
  * bad block info is on bytes 0 and 1
  * the bytes have to be consecutives to avoid
@@ -111,6 +115,7 @@ struct atmel_nfc {
 	/* Point to the sram bank which include readed data via NFC */
 	void			*data_in_sram;
 	bool			will_write_sram;
+	uint32_t		rb_edge;
 };
 static struct atmel_nfc	nand_nfc;
 
@@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
 		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
 		ret = IRQ_HANDLED;
 	}
-	if (pending & NFC_SR_RB_EDGE) {
+	if (pending & host->nfc->rb_edge) {
 		complete(&host->nfc->comp_ready);
-		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
+		nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
 		ret = IRQ_HANDLED;
 	}
 	if (pending & NFC_SR_CMD_DONE) {
@@ -1695,7 +1700,7 @@ static void nfc_prepare_interrupt(struct atmel_nand_host *host, u32 flag)
 	if (flag & NFC_SR_XFR_DONE)
 		init_completion(&host->nfc->comp_xfer_done);
 
-	if (flag & NFC_SR_RB_EDGE)
+	if (flag & host->nfc->rb_edge)
 		init_completion(&host->nfc->comp_ready);
 
 	if (flag & NFC_SR_CMD_DONE)
@@ -1713,7 +1718,7 @@ static int nfc_wait_interrupt(struct atmel_nand_host *host, u32 flag)
 	if (flag & NFC_SR_XFR_DONE)
 		comp[index++] = &host->nfc->comp_xfer_done;
 
-	if (flag & NFC_SR_RB_EDGE)
+	if (flag & host->nfc->rb_edge)
 		comp[index++] = &host->nfc->comp_ready;
 
 	if (flag & NFC_SR_CMD_DONE)
@@ -1781,7 +1786,7 @@ static int nfc_device_ready(struct mtd_info *mtd)
 		dev_err(host->dev, "Lost the interrupt flags: 0x%08x\n",
 				mask & status);
 
-	return status & NFC_SR_RB_EDGE;
+	return status & host->nfc->rb_edge;
 }
 
 static void nfc_select_chip(struct mtd_info *mtd, int chip)
@@ -1954,8 +1959,8 @@ static void nfc_nand_command(struct mtd_info *mtd, unsigned int command,
 		}
 		/* fall through */
 	default:
-		nfc_prepare_interrupt(host, NFC_SR_RB_EDGE);
-		nfc_wait_interrupt(host, NFC_SR_RB_EDGE);
+		nfc_prepare_interrupt(host, host->nfc->rb_edge);
+		nfc_wait_interrupt(host, host->nfc->rb_edge);
 	}
 }
 
@@ -2318,9 +2323,12 @@ static const struct of_device_id atmel_nand_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids);
 
+static const struct of_device_id atmel_nand_nfc_match[];
+
 static int atmel_nand_nfc_probe(struct platform_device *pdev)
 {
 	struct atmel_nfc *nfc = &nand_nfc;
+	const struct atmel_nand_nfc_priv *priv;
 	struct resource *nfc_cmd_regs, *nfc_hsmc_regs, *nfc_sram;
 	int ret;
 
@@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
 		}
 	}
 
+	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;
+	if (NULL == priv)
+		return -ENODEV;
+
+	nfc->rb_edge = priv->rb_edge;
+
 	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
 	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
 
@@ -2380,8 +2394,17 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct atmel_nand_nfc_priv sama5d3_nfc_priv = {
+	.rb_edge = NFC_SR_RB_EDGE0,
+};
+
+static struct atmel_nand_nfc_priv sama5d4_nfc_priv = {
+	.rb_edge = NFC_SR_RB_EDGE3,
+};
+
 static const struct of_device_id atmel_nand_nfc_match[] = {
-	{ .compatible = "atmel,sama5d3-nfc" },
+	{ .compatible = "atmel,sama5d3-nfc", .data = &sama5d3_nfc_priv },
+	{ .compatible = "atmel,sama5d4-nfc", .data = &sama5d4_nfc_priv },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
index 4d5d26221a7e..2cd9c57b1e53 100644
--- a/drivers/mtd/nand/atmel_nand_nfc.h
+++ b/drivers/mtd/nand/atmel_nand_nfc.h
@@ -42,7 +42,10 @@
 #define		NFC_SR_UNDEF		(1 << 21)
 #define		NFC_SR_AWB		(1 << 22)
 #define		NFC_SR_ASE		(1 << 23)
-#define		NFC_SR_RB_EDGE		(1 << 24)
+#define		NFC_SR_RB_EDGE0		(1 << 24)
+#define		NFC_SR_RB_EDGE1		(1 << 25)
+#define		NFC_SR_RB_EDGE2		(1 << 26)
+#define		NFC_SR_RB_EDGE3		(1 << 27)
 
 #define ATMEL_HSMC_NFC_IER	0x0c
 #define ATMEL_HSMC_NFC_IDR	0x10
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Romain Izard]

The NFC controller used to accelerate the NAND transfers on SAMA5 chips
can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.

Use the controller's compatible string to select the correct bit.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
Reviewed-by: Wenyou Yang <Wenyou.yang@atmel.com>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
 drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
 drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 7d4c8eb775a5..89b0db9801b0 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -34,7 +34,7 @@ Optional properties:
 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
 - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
   - Required properties:
-    - compatible : "atmel,sama5d3-nfc".
+    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
     - reg : should specify the address and size used for NFC command registers,
             NFC registers and NFC Sram. NFC Sram address and size can be absent
             if don't want to use it.
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 9d71f9e6a8de..e5d7e7e63f49 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -67,6 +67,10 @@ struct atmel_nand_caps {
 	bool pmecc_correct_erase_page;
 };
 
+struct atmel_nand_nfc_priv {
+	uint32_t rb_edge;
+};
+
 /* oob layout for large page size
  * bad block info is on bytes 0 and 1
  * the bytes have to be consecutives to avoid
@@ -111,6 +115,7 @@ struct atmel_nfc {
 	/* Point to the sram bank which include readed data via NFC */
 	void			*data_in_sram;
 	bool			will_write_sram;
+	uint32_t		rb_edge;
 };
 static struct atmel_nfc	nand_nfc;
 
@@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
 		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
 		ret = IRQ_HANDLED;
 	}
-	if (pending & NFC_SR_RB_EDGE) {
+	if (pending & host->nfc->rb_edge) {
 		complete(&host->nfc->comp_ready);
-		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
+		nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
 		ret = IRQ_HANDLED;
 	}
 	if (pending & NFC_SR_CMD_DONE) {
@@ -1695,7 +1700,7 @@ static void nfc_prepare_interrupt(struct atmel_nand_host *host, u32 flag)
 	if (flag & NFC_SR_XFR_DONE)
 		init_completion(&host->nfc->comp_xfer_done);
 
-	if (flag & NFC_SR_RB_EDGE)
+	if (flag & host->nfc->rb_edge)
 		init_completion(&host->nfc->comp_ready);
 
 	if (flag & NFC_SR_CMD_DONE)
@@ -1713,7 +1718,7 @@ static int nfc_wait_interrupt(struct atmel_nand_host *host, u32 flag)
 	if (flag & NFC_SR_XFR_DONE)
 		comp[index++] = &host->nfc->comp_xfer_done;
 
-	if (flag & NFC_SR_RB_EDGE)
+	if (flag & host->nfc->rb_edge)
 		comp[index++] = &host->nfc->comp_ready;
 
 	if (flag & NFC_SR_CMD_DONE)
@@ -1781,7 +1786,7 @@ static int nfc_device_ready(struct mtd_info *mtd)
 		dev_err(host->dev, "Lost the interrupt flags: 0x%08x\n",
 				mask & status);
 
-	return status & NFC_SR_RB_EDGE;
+	return status & host->nfc->rb_edge;
 }
 
 static void nfc_select_chip(struct mtd_info *mtd, int chip)
@@ -1954,8 +1959,8 @@ static void nfc_nand_command(struct mtd_info *mtd, unsigned int command,
 		}
 		/* fall through */
 	default:
-		nfc_prepare_interrupt(host, NFC_SR_RB_EDGE);
-		nfc_wait_interrupt(host, NFC_SR_RB_EDGE);
+		nfc_prepare_interrupt(host, host->nfc->rb_edge);
+		nfc_wait_interrupt(host, host->nfc->rb_edge);
 	}
 }
 
@@ -2318,9 +2323,12 @@ static const struct of_device_id atmel_nand_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids);
 
+static const struct of_device_id atmel_nand_nfc_match[];
+
 static int atmel_nand_nfc_probe(struct platform_device *pdev)
 {
 	struct atmel_nfc *nfc = &nand_nfc;
+	const struct atmel_nand_nfc_priv *priv;
 	struct resource *nfc_cmd_regs, *nfc_hsmc_regs, *nfc_sram;
 	int ret;
 
@@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
 		}
 	}
 
+	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;
+	if (NULL == priv)
+		return -ENODEV;
+
+	nfc->rb_edge = priv->rb_edge;
+
 	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
 	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
 
@@ -2380,8 +2394,17 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct atmel_nand_nfc_priv sama5d3_nfc_priv = {
+	.rb_edge = NFC_SR_RB_EDGE0,
+};
+
+static struct atmel_nand_nfc_priv sama5d4_nfc_priv = {
+	.rb_edge = NFC_SR_RB_EDGE3,
+};
+
 static const struct of_device_id atmel_nand_nfc_match[] = {
-	{ .compatible = "atmel,sama5d3-nfc" },
+	{ .compatible = "atmel,sama5d3-nfc", .data = &sama5d3_nfc_priv },
+	{ .compatible = "atmel,sama5d4-nfc", .data = &sama5d4_nfc_priv },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
index 4d5d26221a7e..2cd9c57b1e53 100644
--- a/drivers/mtd/nand/atmel_nand_nfc.h
+++ b/drivers/mtd/nand/atmel_nand_nfc.h
@@ -42,7 +42,10 @@
 #define		NFC_SR_UNDEF		(1 << 21)
 #define		NFC_SR_AWB		(1 << 22)
 #define		NFC_SR_ASE		(1 << 23)
-#define		NFC_SR_RB_EDGE		(1 << 24)
+#define		NFC_SR_RB_EDGE0		(1 << 24)
+#define		NFC_SR_RB_EDGE1		(1 << 25)
+#define		NFC_SR_RB_EDGE2		(1 << 26)
+#define		NFC_SR_RB_EDGE3		(1 << 27)
 
 #define ATMEL_HSMC_NFC_IER	0x0c
 #define ATMEL_HSMC_NFC_IDR	0x10
-- 
2.5.0

[PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Romain Izard]

Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
controller that can correct 32 bits in each sector. This controller is
not 100% compatible with the previous revision that corrected a maximum
of 24 bits by sector, as some register addresses overlap.

Using information from the device tree, we can configure the driver to
work with both versions.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
 drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
 drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 89b0db9801b0..90887b430f03 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -1,7 +1,10 @@
 Atmel NAND flash
 
 Required properties:
-- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
+- compatible: The possible values are:
+	"atmel,at91rm9200-nand"
+	"atmel,sama5d2-nand"
+	"atmel,sama5d4-nand"
 - reg : should specify localbus address and size used for the chip,
 	and hardware ECC controller if available.
 	If the hardware ECC is PMECC, it should contain address and size for
@@ -22,7 +25,7 @@ Optional properties:
   Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
   "soft_bch".
 - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
-  Only supported by at91sam9x5 or later sam9 product.
+  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
 - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
   Controller. Supported values are: 2, 4, 8, 12, 24.
 - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index e5d7e7e63f49..6fe50e2d291f 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
 
 struct atmel_nand_caps {
 	bool pmecc_correct_erase_page;
+	uint8_t pmecc_max_correction;
 };
 
 struct atmel_nand_nfc_priv {
@@ -146,6 +147,7 @@ struct atmel_nand_host {
 	int			pmecc_cw_len;	/* Length of codeword */
 
 	void __iomem		*pmerrloc_base;
+	void __iomem		*pmerrloc_el_base;
 	void __iomem		*pmecc_rom_base;
 
 	/* lookup table for alpha_to and index_of */
@@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 	sector_size = host->pmecc_sector_size;
 
 	while (err_nbr) {
-		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
+		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
 		byte_pos = tmp / 8;
 		bit_pos  = tmp % 8;
 
@@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
 		err_no = PTR_ERR(host->pmerrloc_base);
 		goto err;
 	}
+	host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
+		(host->caps->pmecc_max_correction + 1) * 4;
 
 	if (!host->has_no_lookup_table) {
 		regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
@@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/*
+ * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
+ * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
+ * devices from the SAM9 family that have those.
+ */
 static struct atmel_nand_caps at91rm9200_caps = {
 	.pmecc_correct_erase_page = false,
+	.pmecc_max_correction = 24,
 };
 
 static struct atmel_nand_caps sama5d4_caps = {
 	.pmecc_correct_erase_page = true,
+	.pmecc_max_correction = 24,
+};
+
+/*
+ * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
+ * as the increased correction strength requires more registers.
+ */
+static struct atmel_nand_caps sama5d2_caps = {
+	.pmecc_correct_erase_page = true,
+	.pmecc_max_correction = 32,
 };
 
 static const struct of_device_id atmel_nand_dt_ids[] = {
 	{ .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
 	{ .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
+	{ .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
 	{ /* sentinel */ }
 };
 
diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
index 668e7358f19b..ec964c43c932 100644
--- a/drivers/mtd/nand/atmel_nand_ecc.h
+++ b/drivers/mtd/nand/atmel_nand_ecc.h
@@ -108,7 +108,11 @@
 #define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
 #define		PMERRLOC_CALC_DONE		(1 << 0)
 #define ATMEL_PMERRLOC_SIGMAx		0x028	/* Error location SIGMA x */
-#define ATMEL_PMERRLOC_ELx		0x08c	/* Error location x */
+
+/*
+ * The ATMEL_PMERRLOC_ELx register location depends from the number of
+ * bits corrected by the PMECC controller. Do not use it.
+ */
 
 /* Register access macros for PMECC */
 #define pmecc_readl_relaxed(addr, reg) \
@@ -136,7 +140,7 @@
 	readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
 
 #define pmerrloc_readl_el_relaxed(addr, n) \
-	readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
+	readl_relaxed((addr) + ((n) * 4))
 
 /* Galois field dimension */
 #define PMECC_GF_DIMENSION_13			13
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Romain Izard]

Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
controller that can correct 32 bits in each sector. This controller is
not 100% compatible with the previous revision that corrected a maximum
of 24 bits by sector, as some register addresses overlap.

Using information from the device tree, we can configure the driver to
work with both versions.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
 drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
 drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 89b0db9801b0..90887b430f03 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -1,7 +1,10 @@
 Atmel NAND flash
 
 Required properties:
-- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
+- compatible: The possible values are:
+	"atmel,at91rm9200-nand"
+	"atmel,sama5d2-nand"
+	"atmel,sama5d4-nand"
 - reg : should specify localbus address and size used for the chip,
 	and hardware ECC controller if available.
 	If the hardware ECC is PMECC, it should contain address and size for
@@ -22,7 +25,7 @@ Optional properties:
   Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
   "soft_bch".
 - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
-  Only supported by at91sam9x5 or later sam9 product.
+  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
 - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
   Controller. Supported values are: 2, 4, 8, 12, 24.
 - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index e5d7e7e63f49..6fe50e2d291f 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
 
 struct atmel_nand_caps {
 	bool pmecc_correct_erase_page;
+	uint8_t pmecc_max_correction;
 };
 
 struct atmel_nand_nfc_priv {
@@ -146,6 +147,7 @@ struct atmel_nand_host {
 	int			pmecc_cw_len;	/* Length of codeword */
 
 	void __iomem		*pmerrloc_base;
+	void __iomem		*pmerrloc_el_base;
 	void __iomem		*pmecc_rom_base;
 
 	/* lookup table for alpha_to and index_of */
@@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
 	sector_size = host->pmecc_sector_size;
 
 	while (err_nbr) {
-		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
+		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
 		byte_pos = tmp / 8;
 		bit_pos  = tmp % 8;
 
@@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
 		err_no = PTR_ERR(host->pmerrloc_base);
 		goto err;
 	}
+	host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
+		(host->caps->pmecc_max_correction + 1) * 4;
 
 	if (!host->has_no_lookup_table) {
 		regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
@@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/*
+ * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
+ * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
+ * devices from the SAM9 family that have those.
+ */
 static struct atmel_nand_caps at91rm9200_caps = {
 	.pmecc_correct_erase_page = false,
+	.pmecc_max_correction = 24,
 };
 
 static struct atmel_nand_caps sama5d4_caps = {
 	.pmecc_correct_erase_page = true,
+	.pmecc_max_correction = 24,
+};
+
+/*
+ * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
+ * as the increased correction strength requires more registers.
+ */
+static struct atmel_nand_caps sama5d2_caps = {
+	.pmecc_correct_erase_page = true,
+	.pmecc_max_correction = 32,
 };
 
 static const struct of_device_id atmel_nand_dt_ids[] = {
 	{ .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
 	{ .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
+	{ .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
 	{ /* sentinel */ }
 };
 
diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
index 668e7358f19b..ec964c43c932 100644
--- a/drivers/mtd/nand/atmel_nand_ecc.h
+++ b/drivers/mtd/nand/atmel_nand_ecc.h
@@ -108,7 +108,11 @@
 #define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
 #define		PMERRLOC_CALC_DONE		(1 << 0)
 #define ATMEL_PMERRLOC_SIGMAx		0x028	/* Error location SIGMA x */
-#define ATMEL_PMERRLOC_ELx		0x08c	/* Error location x */
+
+/*
+ * The ATMEL_PMERRLOC_ELx register location depends from the number of
+ * bits corrected by the PMECC controller. Do not use it.
+ */
 
 /* Register access macros for PMECC */
 #define pmecc_readl_relaxed(addr, reg) \
@@ -136,7 +140,7 @@
 	readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
 
 #define pmerrloc_readl_el_relaxed(addr, n) \
-	readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
+	readl_relaxed((addr) + ((n) * 4))
 
 /* Galois field dimension */
 #define PMECC_GF_DIMENSION_13			13
-- 
2.5.0

[PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Romain Izard]

As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
as a valid setting when required by the device tree or the NAND
parameter page.

Then configure the controller to do use this new setting.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-
 drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
 drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 90887b430f03..ef0db8e2a0fd 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -27,7 +27,8 @@ Optional properties:
 - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
   Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
 - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
-  Controller. Supported values are: 2, 4, 8, 12, 24.
+  Controller. Supported values are: 2, 4, 8, 12, 24. SAMA5D2 also supports
+  32.
 - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
   are: 512, 1024.
 - atmel,pmecc-lookup-table-offset : includes two offsets of lookup table in ROM
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 6fe50e2d291f..920a0a315a60 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -474,6 +474,7 @@ static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
  *                8-bits                13-bytes                 14-bytes
  *               12-bits                20-bytes                 21-bytes
  *               24-bits                39-bytes                 42-bytes
+ *               32-bits                52-bytes                 56-bytes
  */
 static int pmecc_get_ecc_bytes(int cap, int sector_size)
 {
@@ -1022,6 +1023,9 @@ static void atmel_pmecc_core_init(struct mtd_info *mtd)
 	case 24:
 		val = PMECC_CFG_BCH_ERR24;
 		break;
+	case 32:
+		val = PMECC_CFG_BCH_ERR32;
+		break;
 	}
 
 	if (host->pmecc_sector_size == 512)
@@ -1083,6 +1087,9 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
 
 	/* If device tree doesn't specify, use NAND's minimum ECC parameters */
 	if (host->pmecc_corr_cap == 0) {
+		if (*cap > host->caps->pmecc_max_correction)
+			return -EINVAL;
+
 		/* use the most fitable ecc bits (the near bigger one ) */
 		if (*cap <= 2)
 			host->pmecc_corr_cap = 2;
@@ -1094,6 +1101,8 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
 			host->pmecc_corr_cap = 12;
 		else if (*cap <= 24)
 			host->pmecc_corr_cap = 24;
+		else if (*cap <= 32)
+			host->pmecc_corr_cap = 32;
 		else
 			return -EINVAL;
 	}
@@ -1554,10 +1563,16 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 	 * them from NAND ONFI parameters.
 	 */
 	if (of_property_read_u32(np, "atmel,pmecc-cap", &val) == 0) {
-		if ((val != 2) && (val != 4) && (val != 8) && (val != 12) &&
-				(val != 24)) {
+		if (val > host->caps->pmecc_max_correction) {
+			dev_err(host->dev,
+				"Required ECC strength too high: %u max %u\n",
+				val, host->caps->pmecc_max_correction);
+			return -EINVAL;
+		}
+		if ((val != 2)  && (val != 4)  && (val != 8) &&
+		    (val != 12) && (val != 24) && (val != 32)) {
 			dev_err(host->dev,
-				"Unsupported PMECC correction capability: %d; should be 2, 4, 8, 12 or 24\n",
+				"Required ECC strength not supported: %u\n",
 				val);
 			return -EINVAL;
 		}
@@ -1567,7 +1582,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 	if (of_property_read_u32(np, "atmel,pmecc-sector-size", &val) == 0) {
 		if ((val != 512) && (val != 1024)) {
 			dev_err(host->dev,
-				"Unsupported PMECC sector size: %d; should be 512 or 1024 bytes\n",
+				"Required ECC sector size not supported: %u\n",
 				val);
 			return -EINVAL;
 		}
diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
index ec964c43c932..834d694487bd 100644
--- a/drivers/mtd/nand/atmel_nand_ecc.h
+++ b/drivers/mtd/nand/atmel_nand_ecc.h
@@ -43,6 +43,7 @@
 #define		PMECC_CFG_BCH_ERR8		(2 << 0)
 #define		PMECC_CFG_BCH_ERR12		(3 << 0)
 #define		PMECC_CFG_BCH_ERR24		(4 << 0)
+#define		PMECC_CFG_BCH_ERR32		(5 << 0)
 
 #define		PMECC_CFG_SECTOR512		(0 << 4)
 #define		PMECC_CFG_SECTOR1024		(1 << 4)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Romain Izard]

As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
as a valid setting when required by the device tree or the NAND
parameter page.

Then configure the controller to do use this new setting.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-
 drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
 drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 90887b430f03..ef0db8e2a0fd 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -27,7 +27,8 @@ Optional properties:
 - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
   Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
 - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
-  Controller. Supported values are: 2, 4, 8, 12, 24.
+  Controller. Supported values are: 2, 4, 8, 12, 24. SAMA5D2 also supports
+  32.
 - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
   are: 512, 1024.
 - atmel,pmecc-lookup-table-offset : includes two offsets of lookup table in ROM
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 6fe50e2d291f..920a0a315a60 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -474,6 +474,7 @@ static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
  *                8-bits                13-bytes                 14-bytes
  *               12-bits                20-bytes                 21-bytes
  *               24-bits                39-bytes                 42-bytes
+ *               32-bits                52-bytes                 56-bytes
  */
 static int pmecc_get_ecc_bytes(int cap, int sector_size)
 {
@@ -1022,6 +1023,9 @@ static void atmel_pmecc_core_init(struct mtd_info *mtd)
 	case 24:
 		val = PMECC_CFG_BCH_ERR24;
 		break;
+	case 32:
+		val = PMECC_CFG_BCH_ERR32;
+		break;
 	}
 
 	if (host->pmecc_sector_size == 512)
@@ -1083,6 +1087,9 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
 
 	/* If device tree doesn't specify, use NAND's minimum ECC parameters */
 	if (host->pmecc_corr_cap == 0) {
+		if (*cap > host->caps->pmecc_max_correction)
+			return -EINVAL;
+
 		/* use the most fitable ecc bits (the near bigger one ) */
 		if (*cap <= 2)
 			host->pmecc_corr_cap = 2;
@@ -1094,6 +1101,8 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
 			host->pmecc_corr_cap = 12;
 		else if (*cap <= 24)
 			host->pmecc_corr_cap = 24;
+		else if (*cap <= 32)
+			host->pmecc_corr_cap = 32;
 		else
 			return -EINVAL;
 	}
@@ -1554,10 +1563,16 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 	 * them from NAND ONFI parameters.
 	 */
 	if (of_property_read_u32(np, "atmel,pmecc-cap", &val) == 0) {
-		if ((val != 2) && (val != 4) && (val != 8) && (val != 12) &&
-				(val != 24)) {
+		if (val > host->caps->pmecc_max_correction) {
+			dev_err(host->dev,
+				"Required ECC strength too high: %u max %u\n",
+				val, host->caps->pmecc_max_correction);
+			return -EINVAL;
+		}
+		if ((val != 2)  && (val != 4)  && (val != 8) &&
+		    (val != 12) && (val != 24) && (val != 32)) {
 			dev_err(host->dev,
-				"Unsupported PMECC correction capability: %d; should be 2, 4, 8, 12 or 24\n",
+				"Required ECC strength not supported: %u\n",
 				val);
 			return -EINVAL;
 		}
@@ -1567,7 +1582,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 	if (of_property_read_u32(np, "atmel,pmecc-sector-size", &val) == 0) {
 		if ((val != 512) && (val != 1024)) {
 			dev_err(host->dev,
-				"Unsupported PMECC sector size: %d; should be 512 or 1024 bytes\n",
+				"Required ECC sector size not supported: %u\n",
 				val);
 			return -EINVAL;
 		}
diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
index ec964c43c932..834d694487bd 100644
--- a/drivers/mtd/nand/atmel_nand_ecc.h
+++ b/drivers/mtd/nand/atmel_nand_ecc.h
@@ -43,6 +43,7 @@
 #define		PMECC_CFG_BCH_ERR8		(2 << 0)
 #define		PMECC_CFG_BCH_ERR12		(3 << 0)
 #define		PMECC_CFG_BCH_ERR24		(4 << 0)
+#define		PMECC_CFG_BCH_ERR32		(5 << 0)
 
 #define		PMECC_CFG_SECTOR512		(0 << 4)
 #define		PMECC_CFG_SECTOR1024		(1 << 4)
-- 
2.5.0

[PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

[Romain Izard]

Both nodes are required to access NAND Flash memory. Additional
settings will be necessary at the board level to use it.

Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sama5d2.dtsi | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index aee571448342..80420177ec1a 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -265,6 +265,44 @@
 			cache-level = <2>;
 		};
 
+		nand0: nand@80000000 {
+			compatible = "atmel,sama5d2-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = < /* EBI CS3 */
+				0x80000000 0x08000000
+				/* SMC PMECC regs */
+				0xf8014070 0x00000490
+				/* SMC PMECC Error Location regs */
+				0xf8014500 0x00000200
+				/* ROM Galois tables */
+				0x00040000 0x00018000
+				>;
+			interrupts = <17 IRQ_TYPE_LEVEL_HIGH 6>;
+			atmel,nand-addr-offset = <21>;
+			atmel,nand-cmd-offset = <22>;
+			atmel,nand-has-dma;
+			atmel,has-pmecc;
+			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
+			status = "disabled";
+
+			nfc@90000000 {
+				compatible = "atmel,sama5d4-nfc";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = < /* NFC Command Registers */
+					0xC0000000 0x08000000
+					/* NFC HSMC regs */
+					0xf8014000 0x00000070
+					/* NFC SRAM banks */
+					0x00100000 0x00100000
+					>;
+				clocks = <&hsmc_clk>;
+				atmel,write-by-sram;
+			};
+		};
+
 		sdmmc0: sdio-host@a0000000 {
 			compatible = "atmel,sama5d2-sdhci";
 			reg = <0xa0000000 0x300>;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

[Romain Izard]

Both nodes are required to access NAND Flash memory. Additional
settings will be necessary at the board level to use it.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 arch/arm/boot/dts/sama5d2.dtsi | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index aee571448342..80420177ec1a 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -265,6 +265,44 @@
 			cache-level = <2>;
 		};
 
+		nand0: nand@80000000 {
+			compatible = "atmel,sama5d2-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = < /* EBI CS3 */
+				0x80000000 0x08000000
+				/* SMC PMECC regs */
+				0xf8014070 0x00000490
+				/* SMC PMECC Error Location regs */
+				0xf8014500 0x00000200
+				/* ROM Galois tables */
+				0x00040000 0x00018000
+				>;
+			interrupts = <17 IRQ_TYPE_LEVEL_HIGH 6>;
+			atmel,nand-addr-offset = <21>;
+			atmel,nand-cmd-offset = <22>;
+			atmel,nand-has-dma;
+			atmel,has-pmecc;
+			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
+			status = "disabled";
+
+			nfc@90000000 {
+				compatible = "atmel,sama5d4-nfc";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = < /* NFC Command Registers */
+					0xC0000000 0x08000000
+					/* NFC HSMC regs */
+					0xf8014000 0x00000070
+					/* NFC SRAM banks */
+					0x00100000 0x00100000
+					>;
+				clocks = <&hsmc_clk>;
+				atmel,write-by-sram;
+			};
+		};
+
 		sdmmc0: sdio-host@a0000000 {
 			compatible = "atmel,sama5d2-sdhci";
 			reg = <0xa0000000 0x300>;
-- 
2.5.0

Re: [PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

[Nicolas Ferre]

Le 13/01/2016 17:34, Romain Izard a écrit :
> Both nodes are required to access NAND Flash memory. Additional
> settings will be necessary at the board level to use it.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/boot/dts/sama5d2.dtsi | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index aee571448342..80420177ec1a 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -265,6 +265,44 @@
>  			cache-level = <2>;
>  		};
>  
> +		nand0: nand@80000000 {
> +			compatible = "atmel,sama5d2-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			reg = < /* EBI CS3 */
> +				0x80000000 0x08000000
> +				/* SMC PMECC regs */
> +				0xf8014070 0x00000490
> +				/* SMC PMECC Error Location regs */
> +				0xf8014500 0x00000200
> +				/* ROM Galois tables */
> +				0x00040000 0x00018000
> +				>;
> +			interrupts = <17 IRQ_TYPE_LEVEL_HIGH 6>;
> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			atmel,has-pmecc;
> +			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
> +			status = "disabled";
> +
> +			nfc@90000000 {

It's nfc@c0000000

> +				compatible = "atmel,sama5d4-nfc";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = < /* NFC Command Registers */
> +					0xC0000000 0x08000000

Lower case please

> +					/* NFC HSMC regs */
> +					0xf8014000 0x00000070
> +					/* NFC SRAM banks */
> +					0x00100000 0x00100000
> +					>;
> +				clocks = <&hsmc_clk>;
> +				atmel,write-by-sram;
> +			};
> +		};
> +

Otherwise, it seems okay. When corrected, you can add my:
Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Brian, Wenyou,
I'm okay if this patch goes to Mainline with the mtd subsystem: there
should be no conflict with arm-soc for the upcoming kernel revisions.

Thanks, bye.


>  		sdmmc0: sdio-host@a0000000 {
>  			compatible = "atmel,sama5d2-sdhci";
>  			reg = <0xa0000000 0x300>;
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

[Nicolas Ferre]

Le 13/01/2016 17:34, Romain Izard a écrit :
> Both nodes are required to access NAND Flash memory. Additional
> settings will be necessary at the board level to use it.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  arch/arm/boot/dts/sama5d2.dtsi | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index aee571448342..80420177ec1a 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -265,6 +265,44 @@
>  			cache-level = <2>;
>  		};
>  
> +		nand0: nand@80000000 {
> +			compatible = "atmel,sama5d2-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			reg = < /* EBI CS3 */
> +				0x80000000 0x08000000
> +				/* SMC PMECC regs */
> +				0xf8014070 0x00000490
> +				/* SMC PMECC Error Location regs */
> +				0xf8014500 0x00000200
> +				/* ROM Galois tables */
> +				0x00040000 0x00018000
> +				>;
> +			interrupts = <17 IRQ_TYPE_LEVEL_HIGH 6>;
> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			atmel,has-pmecc;
> +			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
> +			status = "disabled";
> +
> +			nfc@90000000 {

It's nfc@c0000000

> +				compatible = "atmel,sama5d4-nfc";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = < /* NFC Command Registers */
> +					0xC0000000 0x08000000

Lower case please

> +					/* NFC HSMC regs */
> +					0xf8014000 0x00000070
> +					/* NFC SRAM banks */
> +					0x00100000 0x00100000
> +					>;
> +				clocks = <&hsmc_clk>;
> +				atmel,write-by-sram;
> +			};
> +		};
> +

Otherwise, it seems okay. When corrected, you can add my:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Brian, Wenyou,
I'm okay if this patch goes to Mainline with the mtd subsystem: there
should be no conflict with arm-soc for the upcoming kernel revisions.

Thanks, bye.


>  		sdmmc0: sdio-host@a0000000 {
>  			compatible = "atmel,sama5d2-sdhci";
>  			reg = <0xa0000000 0x300>;
> 


-- 
Nicolas Ferre

[PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes

[Nicolas Ferre]

Le 13/01/2016 17:34, Romain Izard a ?crit :
> Both nodes are required to access NAND Flash memory. Additional
> settings will be necessary at the board level to use it.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  arch/arm/boot/dts/sama5d2.dtsi | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index aee571448342..80420177ec1a 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -265,6 +265,44 @@
>  			cache-level = <2>;
>  		};
>  
> +		nand0: nand at 80000000 {
> +			compatible = "atmel,sama5d2-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			reg = < /* EBI CS3 */
> +				0x80000000 0x08000000
> +				/* SMC PMECC regs */
> +				0xf8014070 0x00000490
> +				/* SMC PMECC Error Location regs */
> +				0xf8014500 0x00000200
> +				/* ROM Galois tables */
> +				0x00040000 0x00018000
> +				>;
> +			interrupts = <17 IRQ_TYPE_LEVEL_HIGH 6>;
> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			atmel,has-pmecc;
> +			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
> +			status = "disabled";
> +
> +			nfc at 90000000 {

It's nfc at c0000000

> +				compatible = "atmel,sama5d4-nfc";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = < /* NFC Command Registers */
> +					0xC0000000 0x08000000

Lower case please

> +					/* NFC HSMC regs */
> +					0xf8014000 0x00000070
> +					/* NFC SRAM banks */
> +					0x00100000 0x00100000
> +					>;
> +				clocks = <&hsmc_clk>;
> +				atmel,write-by-sram;
> +			};
> +		};
> +

Otherwise, it seems okay. When corrected, you can add my:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Brian, Wenyou,
I'm okay if this patch goes to Mainline with the mtd subsystem: there
should be no conflict with arm-soc for the upcoming kernel revisions.

Thanks, bye.


>  		sdmmc0: sdio-host at a0000000 {
>  			compatible = "atmel,sama5d2-sdhci";
>  			reg = <0xa0000000 0x300>;
> 


-- 
Nicolas Ferre

Re: [PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:13 +0100
Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> When using multi-bit ECC, it is normal for the NAND Flash driver to
> correct bit errors during the life of the product. Those errors will
> only be cleared once a threshold has been reached, and corrections can
> occur regularly before this.
> 
> Use only dev_dbg and not dev_info to report the bitflips, to keep the
> system log clean when everything works correctly.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by:  Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Reviewed-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

> ---
>  drivers/mtd/nand/atmel_nand.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 46010bd895b1..9d71f9e6a8de 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -824,7 +824,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  			*(buf + byte_pos) ^= (1 << bit_pos);
>  
>  			pos = sector_num * host->pmecc_sector_size + byte_pos;
> -			dev_info(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +			dev_dbg(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>  				pos, bit_pos, err_byte, *(buf + byte_pos));
>  		} else {
>  			/* Bit flip in OOB area */
> @@ -834,7 +834,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  			ecc[tmp] ^= (1 << bit_pos);
>  
>  			pos = tmp + nand_chip->ecc.layout->eccpos[0];
> -			dev_info(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +			dev_dbg(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>  				pos, bit_pos, err_byte, ecc[tmp]);
>  		}
>  



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:13 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> When using multi-bit ECC, it is normal for the NAND Flash driver to
> correct bit errors during the life of the product. Those errors will
> only be cleared once a threshold has been reached, and corrections can
> occur regularly before this.
> 
> Use only dev_dbg and not dev_info to report the bitflips, to keep the
> system log clean when everything works correctly.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> Acked-by:  Wenyou Yang <wenyou.yang@atmel.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/nand/atmel_nand.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 46010bd895b1..9d71f9e6a8de 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -824,7 +824,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  			*(buf + byte_pos) ^= (1 << bit_pos);
>  
>  			pos = sector_num * host->pmecc_sector_size + byte_pos;
> -			dev_info(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +			dev_dbg(host->dev, "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>  				pos, bit_pos, err_byte, *(buf + byte_pos));
>  		} else {
>  			/* Bit flip in OOB area */
> @@ -834,7 +834,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  			ecc[tmp] ^= (1 << bit_pos);
>  
>  			pos = tmp + nand_chip->ecc.layout->eccpos[0];
> -			dev_info(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +			dev_dbg(host->dev, "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>  				pos, bit_pos, err_byte, ecc[tmp]);
>  		}
>  



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

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Boris Brezillon]

Hi Romain,

On Wed, 13 Jan 2016 17:34:14 +0100
Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> The NFC controller used to accelerate the NAND transfers on SAMA5 chips
> can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.
> 
> Use the controller's compatible string to select the correct bit.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Wenyou Yang <Wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
>  drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
>  drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
>  3 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 7d4c8eb775a5..89b0db9801b0 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -34,7 +34,7 @@ Optional properties:
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
>  - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
>    - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> +    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
>      - reg : should specify the address and size used for NFC command registers,
>              NFC registers and NFC Sram. NFC Sram address and size can be absent
>              if don't want to use it.
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 9d71f9e6a8de..e5d7e7e63f49 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
>  	bool pmecc_correct_erase_page;
>  };
>  
> +struct atmel_nand_nfc_priv {

Can you rename this struct into atmel_nfc_caps to be consistant with the
atmel_nand_caps?

> +	uint32_t rb_edge;

Actually, AFAIU, it's not encoding the type of edges, but the available
native R/B lines (by native I mean the R/B lines directly connected to
the NFC IP).
I suggest renaming this field avail_rb_lines, and making it a bitfield
(one bit per possible R/B line).

> +};
> +
>  /* oob layout for large page size
>   * bad block info is on bytes 0 and 1
>   * the bytes have to be consecutives to avoid
> @@ -111,6 +115,7 @@ struct atmel_nfc {
>  	/* Point to the sram bank which include readed data via NFC */
>  	void			*data_in_sram;
>  	bool			will_write_sram;
> +	uint32_t		rb_edge;

Replace this by
	const struct atmel_nfc_caps *caps;

so that next time you have a per-SoC particularity, you can just add it
to your struct atmel_nfc_caps without adding new fields to atmel_nfc.

>  };
>  static struct atmel_nfc	nand_nfc;
>  
> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
>  		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
>  		ret = IRQ_HANDLED;
>  	}
> -	if (pending & NFC_SR_RB_EDGE) {
> +	if (pending & host->nfc->rb_edge) {
>  		complete(&host->nfc->comp_ready);
> -		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
> +		nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);

How about creating a macro that would generate the appropriate bitmask
for you?

Something like

#define NFC_SR_RB_EDGE_MASK(x)	((x) << 24)

>  		ret = IRQ_HANDLED;
>  	}
>  	if (pending & NFC_SR_CMD_DONE) {
> @@ -1695,7 +1700,7 @@ static void nfc_prepare_interrupt(struct atmel_nand_host *host, u32 flag)
>  	if (flag & NFC_SR_XFR_DONE)
>  		init_completion(&host->nfc->comp_xfer_done);
>  
> -	if (flag & NFC_SR_RB_EDGE)
> +	if (flag & host->nfc->rb_edge)
>  		init_completion(&host->nfc->comp_ready);
>  
>  	if (flag & NFC_SR_CMD_DONE)
> @@ -1713,7 +1718,7 @@ static int nfc_wait_interrupt(struct atmel_nand_host *host, u32 flag)
>  	if (flag & NFC_SR_XFR_DONE)
>  		comp[index++] = &host->nfc->comp_xfer_done;
>  
> -	if (flag & NFC_SR_RB_EDGE)
> +	if (flag & host->nfc->rb_edge)
>  		comp[index++] = &host->nfc->comp_ready;
>  
>  	if (flag & NFC_SR_CMD_DONE)
> @@ -1781,7 +1786,7 @@ static int nfc_device_ready(struct mtd_info *mtd)
>  		dev_err(host->dev, "Lost the interrupt flags: 0x%08x\n",
>  				mask & status);
>  
> -	return status & NFC_SR_RB_EDGE;
> +	return status & host->nfc->rb_edge;
>  }
>  
>  static void nfc_select_chip(struct mtd_info *mtd, int chip)
> @@ -1954,8 +1959,8 @@ static void nfc_nand_command(struct mtd_info *mtd, unsigned int command,
>  		}
>  		/* fall through */
>  	default:
> -		nfc_prepare_interrupt(host, NFC_SR_RB_EDGE);
> -		nfc_wait_interrupt(host, NFC_SR_RB_EDGE);
> +		nfc_prepare_interrupt(host, host->nfc->rb_edge);
> +		nfc_wait_interrupt(host, host->nfc->rb_edge);
>  	}
>  }
>  
> @@ -2318,9 +2323,12 @@ static const struct of_device_id atmel_nand_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids);
>  
> +static const struct of_device_id atmel_nand_nfc_match[];
> +
>  static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  {
>  	struct atmel_nfc *nfc = &nand_nfc;
> +	const struct atmel_nand_nfc_priv *priv;
>  	struct resource *nfc_cmd_regs, *nfc_hsmc_regs, *nfc_sram;
>  	int ret;
>  
> @@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;
> +	if (NULL == priv)
> +		return -ENODEV;
> +
> +	nfc->rb_edge = priv->rb_edge;
> +
>  	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
>  	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
>  
> @@ -2380,8 +2394,17 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static struct atmel_nand_nfc_priv sama5d3_nfc_priv = {
> +	.rb_edge = NFC_SR_RB_EDGE0,
> +};
> +
> +static struct atmel_nand_nfc_priv sama5d4_nfc_priv = {
> +	.rb_edge = NFC_SR_RB_EDGE3,
> +};
> +
>  static const struct of_device_id atmel_nand_nfc_match[] = {
> -	{ .compatible = "atmel,sama5d3-nfc" },
> +	{ .compatible = "atmel,sama5d3-nfc", .data = &sama5d3_nfc_priv },
> +	{ .compatible = "atmel,sama5d4-nfc", .data = &sama5d4_nfc_priv },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
> index 4d5d26221a7e..2cd9c57b1e53 100644
> --- a/drivers/mtd/nand/atmel_nand_nfc.h
> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
> @@ -42,7 +42,10 @@
>  #define		NFC_SR_UNDEF		(1 << 21)
>  #define		NFC_SR_AWB		(1 << 22)
>  #define		NFC_SR_ASE		(1 << 23)
> -#define		NFC_SR_RB_EDGE		(1 << 24)
> +#define		NFC_SR_RB_EDGE0		(1 << 24)
> +#define		NFC_SR_RB_EDGE1		(1 << 25)
> +#define		NFC_SR_RB_EDGE2		(1 << 26)
> +#define		NFC_SR_RB_EDGE3		(1 << 27)

Please replace those 4 definitions by:

#define			NFC_SR_RB_EDGE(x)	BIT(x + 24)

>  
>  #define ATMEL_HSMC_NFC_IER	0x0c
>  #define ATMEL_HSMC_NFC_IDR	0x10

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Boris Brezillon]

Hi Romain,

On Wed, 13 Jan 2016 17:34:14 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> The NFC controller used to accelerate the NAND transfers on SAMA5 chips
> can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.
> 
> Use the controller's compatible string to select the correct bit.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> Reviewed-by: Wenyou Yang <Wenyou.yang@atmel.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
>  drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
>  drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
>  3 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 7d4c8eb775a5..89b0db9801b0 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -34,7 +34,7 @@ Optional properties:
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
>  - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
>    - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> +    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
>      - reg : should specify the address and size used for NFC command registers,
>              NFC registers and NFC Sram. NFC Sram address and size can be absent
>              if don't want to use it.
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 9d71f9e6a8de..e5d7e7e63f49 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
>  	bool pmecc_correct_erase_page;
>  };
>  
> +struct atmel_nand_nfc_priv {

Can you rename this struct into atmel_nfc_caps to be consistant with the
atmel_nand_caps?

> +	uint32_t rb_edge;

Actually, AFAIU, it's not encoding the type of edges, but the available
native R/B lines (by native I mean the R/B lines directly connected to
the NFC IP).
I suggest renaming this field avail_rb_lines, and making it a bitfield
(one bit per possible R/B line).

> +};
> +
>  /* oob layout for large page size
>   * bad block info is on bytes 0 and 1
>   * the bytes have to be consecutives to avoid
> @@ -111,6 +115,7 @@ struct atmel_nfc {
>  	/* Point to the sram bank which include readed data via NFC */
>  	void			*data_in_sram;
>  	bool			will_write_sram;
> +	uint32_t		rb_edge;

Replace this by
	const struct atmel_nfc_caps *caps;

so that next time you have a per-SoC particularity, you can just add it
to your struct atmel_nfc_caps without adding new fields to atmel_nfc.

>  };
>  static struct atmel_nfc	nand_nfc;
>  
> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
>  		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
>  		ret = IRQ_HANDLED;
>  	}
> -	if (pending & NFC_SR_RB_EDGE) {
> +	if (pending & host->nfc->rb_edge) {
>  		complete(&host->nfc->comp_ready);
> -		nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
> +		nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);

How about creating a macro that would generate the appropriate bitmask
for you?

Something like

#define NFC_SR_RB_EDGE_MASK(x)	((x) << 24)

>  		ret = IRQ_HANDLED;
>  	}
>  	if (pending & NFC_SR_CMD_DONE) {
> @@ -1695,7 +1700,7 @@ static void nfc_prepare_interrupt(struct atmel_nand_host *host, u32 flag)
>  	if (flag & NFC_SR_XFR_DONE)
>  		init_completion(&host->nfc->comp_xfer_done);
>  
> -	if (flag & NFC_SR_RB_EDGE)
> +	if (flag & host->nfc->rb_edge)
>  		init_completion(&host->nfc->comp_ready);
>  
>  	if (flag & NFC_SR_CMD_DONE)
> @@ -1713,7 +1718,7 @@ static int nfc_wait_interrupt(struct atmel_nand_host *host, u32 flag)
>  	if (flag & NFC_SR_XFR_DONE)
>  		comp[index++] = &host->nfc->comp_xfer_done;
>  
> -	if (flag & NFC_SR_RB_EDGE)
> +	if (flag & host->nfc->rb_edge)
>  		comp[index++] = &host->nfc->comp_ready;
>  
>  	if (flag & NFC_SR_CMD_DONE)
> @@ -1781,7 +1786,7 @@ static int nfc_device_ready(struct mtd_info *mtd)
>  		dev_err(host->dev, "Lost the interrupt flags: 0x%08x\n",
>  				mask & status);
>  
> -	return status & NFC_SR_RB_EDGE;
> +	return status & host->nfc->rb_edge;
>  }
>  
>  static void nfc_select_chip(struct mtd_info *mtd, int chip)
> @@ -1954,8 +1959,8 @@ static void nfc_nand_command(struct mtd_info *mtd, unsigned int command,
>  		}
>  		/* fall through */
>  	default:
> -		nfc_prepare_interrupt(host, NFC_SR_RB_EDGE);
> -		nfc_wait_interrupt(host, NFC_SR_RB_EDGE);
> +		nfc_prepare_interrupt(host, host->nfc->rb_edge);
> +		nfc_wait_interrupt(host, host->nfc->rb_edge);
>  	}
>  }
>  
> @@ -2318,9 +2323,12 @@ static const struct of_device_id atmel_nand_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids);
>  
> +static const struct of_device_id atmel_nand_nfc_match[];
> +
>  static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  {
>  	struct atmel_nfc *nfc = &nand_nfc;
> +	const struct atmel_nand_nfc_priv *priv;
>  	struct resource *nfc_cmd_regs, *nfc_hsmc_regs, *nfc_sram;
>  	int ret;
>  
> @@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;
> +	if (NULL == priv)
> +		return -ENODEV;
> +
> +	nfc->rb_edge = priv->rb_edge;
> +
>  	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
>  	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
>  
> @@ -2380,8 +2394,17 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static struct atmel_nand_nfc_priv sama5d3_nfc_priv = {
> +	.rb_edge = NFC_SR_RB_EDGE0,
> +};
> +
> +static struct atmel_nand_nfc_priv sama5d4_nfc_priv = {
> +	.rb_edge = NFC_SR_RB_EDGE3,
> +};
> +
>  static const struct of_device_id atmel_nand_nfc_match[] = {
> -	{ .compatible = "atmel,sama5d3-nfc" },
> +	{ .compatible = "atmel,sama5d3-nfc", .data = &sama5d3_nfc_priv },
> +	{ .compatible = "atmel,sama5d4-nfc", .data = &sama5d4_nfc_priv },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
> index 4d5d26221a7e..2cd9c57b1e53 100644
> --- a/drivers/mtd/nand/atmel_nand_nfc.h
> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
> @@ -42,7 +42,10 @@
>  #define		NFC_SR_UNDEF		(1 << 21)
>  #define		NFC_SR_AWB		(1 << 22)
>  #define		NFC_SR_ASE		(1 << 23)
> -#define		NFC_SR_RB_EDGE		(1 << 24)
> +#define		NFC_SR_RB_EDGE0		(1 << 24)
> +#define		NFC_SR_RB_EDGE1		(1 << 25)
> +#define		NFC_SR_RB_EDGE2		(1 << 26)
> +#define		NFC_SR_RB_EDGE3		(1 << 27)

Please replace those 4 definitions by:

#define			NFC_SR_RB_EDGE(x)	BIT(x + 24)

>  
>  #define ATMEL_HSMC_NFC_IER	0x0c
>  #define ATMEL_HSMC_NFC_IDR	0x10

Best Regards,

Boris

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

Re: [PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:16PM +0100, Romain Izard wrote:
> As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
> as a valid setting when required by the device tree or the NAND
> parameter page.
> 
> Then configure the controller to do use this new setting.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>  drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:16PM +0100, Romain Izard wrote:
> As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
> as a valid setting when required by the device tree or the NAND
> parameter page.
> 
> Then configure the controller to do use this new setting.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> controller that can correct 32 bits in each sector. This controller is
> not 100% compatible with the previous revision that corrected a maximum
> of 24 bits by sector, as some register addresses overlap.
> 
> Using information from the device tree, we can configure the driver to
> work with both versions.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 89b0db9801b0..90887b430f03 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -1,7 +1,10 @@
>  Atmel NAND flash
>  
>  Required properties:
> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> +- compatible: The possible values are:
> +	"atmel,at91rm9200-nand"
> +	"atmel,sama5d2-nand"
> +	"atmel,sama5d4-nand"
>  - reg : should specify localbus address and size used for the chip,
>  	and hardware ECC controller if available.
>  	If the hardware ECC is PMECC, it should contain address and size for
> @@ -22,7 +25,7 @@ Optional properties:
>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>    "soft_bch".
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> -  Only supported by at91sam9x5 or later sam9 product.
> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.

What compatible string would AT91SAM9x5 be?

>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>    Controller. Supported values are: 2, 4, 8, 12, 24.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> controller that can correct 32 bits in each sector. This controller is
> not 100% compatible with the previous revision that corrected a maximum
> of 24 bits by sector, as some register addresses overlap.
> 
> Using information from the device tree, we can configure the driver to
> work with both versions.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 89b0db9801b0..90887b430f03 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -1,7 +1,10 @@
>  Atmel NAND flash
>  
>  Required properties:
> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> +- compatible: The possible values are:
> +	"atmel,at91rm9200-nand"
> +	"atmel,sama5d2-nand"
> +	"atmel,sama5d4-nand"
>  - reg : should specify localbus address and size used for the chip,
>  	and hardware ECC controller if available.
>  	If the hardware ECC is PMECC, it should contain address and size for
> @@ -22,7 +25,7 @@ Optional properties:
>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>    "soft_bch".
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> -  Only supported by at91sam9x5 or later sam9 product.
> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.

What compatible string would AT91SAM9x5 be?

>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>    Controller. Supported values are: 2, 4, 8, 12, 24.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values

RE: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Yang, Wenyou]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2016年1月14日 9:13
> To: Romain Izard <romain.izard.pro@gmail.com>
> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou
> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>
> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> 
> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> > controller that can correct 32 bits in each sector. This controller is
> > not 100% compatible with the previous revision that corrected a
> > maximum of 24 bits by sector, as some register addresses overlap.
> >
> > Using information from the device tree, we can configure the driver to
> > work with both versions.
> >
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > ---
> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > index 89b0db9801b0..90887b430f03 100644
> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > @@ -1,7 +1,10 @@
> >  Atmel NAND flash
> >
> >  Required properties:
> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> > +- compatible: The possible values are:
> > +	"atmel,at91rm9200-nand"
> > +	"atmel,sama5d2-nand"
> > +	"atmel,sama5d4-nand"
> >  - reg : should specify localbus address and size used for the chip,
> >  	and hardware ECC controller if available.
> >  	If the hardware ECC is PMECC, it should contain address and size for
> > @@ -22,7 +25,7 @@ Optional properties:
> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >    "soft_bch".
> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> > -  Only supported by at91sam9x5 or later sam9 product.
> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> 
> What compatible string would AT91SAM9x5 be?

"atmel,at91rm9200-nand".

> 
> >  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
> >    Controller. Supported values are: 2, 4, 8, 12, 24.
> >  - atmel,pmecc-sector-size : sector size for ECC computation.
> > Supported values


Best Regards,
Wenyou Yang

RE: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Yang, Wenyou]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2016年1月14日 9:13
> To: Romain Izard <romain.izard.pro@gmail.com>
> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou
> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>
> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> 
> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> > controller that can correct 32 bits in each sector. This controller is
> > not 100% compatible with the previous revision that corrected a
> > maximum of 24 bits by sector, as some register addresses overlap.
> >
> > Using information from the device tree, we can configure the driver to
> > work with both versions.
> >
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > ---
> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > index 89b0db9801b0..90887b430f03 100644
> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> > @@ -1,7 +1,10 @@
> >  Atmel NAND flash
> >
> >  Required properties:
> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> > +- compatible: The possible values are:
> > +	"atmel,at91rm9200-nand"
> > +	"atmel,sama5d2-nand"
> > +	"atmel,sama5d4-nand"
> >  - reg : should specify localbus address and size used for the chip,
> >  	and hardware ECC controller if available.
> >  	If the hardware ECC is PMECC, it should contain address and size for
> > @@ -22,7 +25,7 @@ Optional properties:
> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >    "soft_bch".
> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> > -  Only supported by at91sam9x5 or later sam9 product.
> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> 
> What compatible string would AT91SAM9x5 be?

"atmel,at91rm9200-nand".

> 
> >  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
> >    Controller. Supported values are: 2, 4, 8, 12, 24.
> >  - atmel,pmecc-sector-size : sector size for ECC computation.
> > Supported values


Best Regards,
Wenyou Yang

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:14PM +0100, Romain Izard wrote:
> The NFC controller used to accelerate the NAND transfers on SAMA5 chips
> can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.
> 
> Use the controller's compatible string to select the correct bit.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Wenyou Yang <Wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
>  drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
>  drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
>  3 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 7d4c8eb775a5..89b0db9801b0 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -34,7 +34,7 @@ Optional properties:
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
>  - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
>    - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> +    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
>      - reg : should specify the address and size used for NFC command registers,
>              NFC registers and NFC Sram. NFC Sram address and size can be absent
>              if don't want to use it.

For the binding: 

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

[...]

> @@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;

Use of_device_get_match_data

> +	if (NULL == priv)
> +		return -ENODEV;
> +
> +	nfc->rb_edge = priv->rb_edge;
> +
>  	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
>  	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
>  
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Rob Herring]

On Wed, Jan 13, 2016 at 05:34:14PM +0100, Romain Izard wrote:
> The NFC controller used to accelerate the NAND transfers on SAMA5 chips
> can use either RB_EDGE0 or RB_EDGE3 as its ready/busy interrupt bit.
> 
> Use the controller's compatible string to select the correct bit.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> Reviewed-by: Wenyou Yang <Wenyou.yang@atmel.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  2 +-
>  drivers/mtd/nand/atmel_nand.c                      | 39 +++++++++++++++++-----
>  drivers/mtd/nand/atmel_nand_nfc.h                  |  5 ++-
>  3 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 7d4c8eb775a5..89b0db9801b0 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -34,7 +34,7 @@ Optional properties:
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
>  - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
>    - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> +    - compatible : "atmel,sama5d3-nfc" or "atmel,sama5d4-nfc".
>      - reg : should specify the address and size used for NFC command registers,
>              NFC registers and NFC Sram. NFC Sram address and size can be absent
>              if don't want to use it.

For the binding: 

Acked-by: Rob Herring <robh@kernel.org>

[...]

> @@ -2352,6 +2360,12 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	priv = of_match_device(atmel_nand_nfc_match, &pdev->dev)->data;

Use of_device_get_match_data

> +	if (NULL == priv)
> +		return -ENODEV;
> +
> +	nfc->rb_edge = priv->rb_edge;
> +
>  	nfc_writel(nfc->hsmc_regs, IDR, 0xffffffff);
>  	nfc_readl(nfc->hsmc_regs, SR);	/* clear the NFC_SR */
>  

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:15 +0100
Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> controller that can correct 32 bits in each sector. This controller is
> not 100% compatible with the previous revision that corrected a maximum
> of 24 bits by sector, as some register addresses overlap.
> 
> Using information from the device tree, we can configure the driver to
> work with both versions.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 89b0db9801b0..90887b430f03 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -1,7 +1,10 @@
>  Atmel NAND flash
>  
>  Required properties:
> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> +- compatible: The possible values are:
> +	"atmel,at91rm9200-nand"
> +	"atmel,sama5d2-nand"
> +	"atmel,sama5d4-nand"
>  - reg : should specify localbus address and size used for the chip,
>  	and hardware ECC controller if available.
>  	If the hardware ECC is PMECC, it should contain address and size for
> @@ -22,7 +25,7 @@ Optional properties:
>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>    "soft_bch".
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> -  Only supported by at91sam9x5 or later sam9 product.
> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>    Controller. Supported values are: 2, 4, 8, 12, 24.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index e5d7e7e63f49..6fe50e2d291f 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
>  
>  struct atmel_nand_caps {
>  	bool pmecc_correct_erase_page;
> +	uint8_t pmecc_max_correction;
>  };
>  
>  struct atmel_nand_nfc_priv {
> @@ -146,6 +147,7 @@ struct atmel_nand_host {
>  	int			pmecc_cw_len;	/* Length of codeword */
>  
>  	void __iomem		*pmerrloc_base;
> +	void __iomem		*pmerrloc_el_base;
>  	void __iomem		*pmecc_rom_base;
>  
>  	/* lookup table for alpha_to and index_of */
> @@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  	sector_size = host->pmecc_sector_size;
>  
>  	while (err_nbr) {
> -		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
> +		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
>  		byte_pos = tmp / 8;
>  		bit_pos  = tmp % 8;
>  
> @@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>  		err_no = PTR_ERR(host->pmerrloc_base);
>  		goto err;
>  	}

How about putting the comment you added below here:

/*
 * The ATMEL_PMERRLOC_ELx register location depends on the number of
 * bits corrected by the PMECC controller. Pre-calculate the
 * pmerrloc_el_base according to the PMECC capabilities.
 */

> +	host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
> +		(host->caps->pmecc_max_correction + 1) * 4;
>  
>  	if (!host->has_no_lookup_table) {
>  		regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> @@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +/*
> + * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
> + * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
> + * devices from the SAM9 family that have those.
> + */
>  static struct atmel_nand_caps at91rm9200_caps = {
>  	.pmecc_correct_erase_page = false,
> +	.pmecc_max_correction = 24,
>  };
>  
>  static struct atmel_nand_caps sama5d4_caps = {
>  	.pmecc_correct_erase_page = true,
> +	.pmecc_max_correction = 24,
> +};
> +
> +/*
> + * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
> + * as the increased correction strength requires more registers.
> + */
> +static struct atmel_nand_caps sama5d2_caps = {
> +	.pmecc_correct_erase_page = true,
> +	.pmecc_max_correction = 32,
>  };
>  
>  static const struct of_device_id atmel_nand_dt_ids[] = {
>  	{ .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
>  	{ .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
> +	{ .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
>  	{ /* sentinel */ }
>  };
>  
> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
> index 668e7358f19b..ec964c43c932 100644
> --- a/drivers/mtd/nand/atmel_nand_ecc.h
> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
> @@ -108,7 +108,11 @@
>  #define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
>  #define		PMERRLOC_CALC_DONE		(1 << 0)
>  #define ATMEL_PMERRLOC_SIGMAx		0x028	/* Error location SIGMA x */
> -#define ATMEL_PMERRLOC_ELx		0x08c	/* Error location x */
> +
> +/*
> + * The ATMEL_PMERRLOC_ELx register location depends from the number of
> + * bits corrected by the PMECC controller. Do not use it.
> + */
>  
>  /* Register access macros for PMECC */
>  #define pmecc_readl_relaxed(addr, reg) \
> @@ -136,7 +140,7 @@
>  	readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
>  
>  #define pmerrloc_readl_el_relaxed(addr, n) \
> -	readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
> +	readl_relaxed((addr) + ((n) * 4))

Another option would have been to pass the maximum ECC strength here,
and modify the ATMEL_PMERRLOC_ELx macro to take a max_strength argument
and return the adjusted ELx offset.
Anyway, both solutions work for me.

>  
>  /* Galois field dimension */
>  #define PMECC_GF_DIMENSION_13			13



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:15 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> controller that can correct 32 bits in each sector. This controller is
> not 100% compatible with the previous revision that corrected a maximum
> of 24 bits by sector, as some register addresses overlap.
> 
> Using information from the device tree, we can configure the driver to
> work with both versions.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 89b0db9801b0..90887b430f03 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -1,7 +1,10 @@
>  Atmel NAND flash
>  
>  Required properties:
> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> +- compatible: The possible values are:
> +	"atmel,at91rm9200-nand"
> +	"atmel,sama5d2-nand"
> +	"atmel,sama5d4-nand"
>  - reg : should specify localbus address and size used for the chip,
>  	and hardware ECC controller if available.
>  	If the hardware ECC is PMECC, it should contain address and size for
> @@ -22,7 +25,7 @@ Optional properties:
>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>    "soft_bch".
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> -  Only supported by at91sam9x5 or later sam9 product.
> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>    Controller. Supported values are: 2, 4, 8, 12, 24.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index e5d7e7e63f49..6fe50e2d291f 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
>  
>  struct atmel_nand_caps {
>  	bool pmecc_correct_erase_page;
> +	uint8_t pmecc_max_correction;
>  };
>  
>  struct atmel_nand_nfc_priv {
> @@ -146,6 +147,7 @@ struct atmel_nand_host {
>  	int			pmecc_cw_len;	/* Length of codeword */
>  
>  	void __iomem		*pmerrloc_base;
> +	void __iomem		*pmerrloc_el_base;
>  	void __iomem		*pmecc_rom_base;
>  
>  	/* lookup table for alpha_to and index_of */
> @@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>  	sector_size = host->pmecc_sector_size;
>  
>  	while (err_nbr) {
> -		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
> +		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
>  		byte_pos = tmp / 8;
>  		bit_pos  = tmp % 8;
>  
> @@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>  		err_no = PTR_ERR(host->pmerrloc_base);
>  		goto err;
>  	}

How about putting the comment you added below here:

/*
 * The ATMEL_PMERRLOC_ELx register location depends on the number of
 * bits corrected by the PMECC controller. Pre-calculate the
 * pmerrloc_el_base according to the PMECC capabilities.
 */

> +	host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
> +		(host->caps->pmecc_max_correction + 1) * 4;
>  
>  	if (!host->has_no_lookup_table) {
>  		regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> @@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +/*
> + * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
> + * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
> + * devices from the SAM9 family that have those.
> + */
>  static struct atmel_nand_caps at91rm9200_caps = {
>  	.pmecc_correct_erase_page = false,
> +	.pmecc_max_correction = 24,
>  };
>  
>  static struct atmel_nand_caps sama5d4_caps = {
>  	.pmecc_correct_erase_page = true,
> +	.pmecc_max_correction = 24,
> +};
> +
> +/*
> + * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
> + * as the increased correction strength requires more registers.
> + */
> +static struct atmel_nand_caps sama5d2_caps = {
> +	.pmecc_correct_erase_page = true,
> +	.pmecc_max_correction = 32,
>  };
>  
>  static const struct of_device_id atmel_nand_dt_ids[] = {
>  	{ .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
>  	{ .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
> +	{ .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
>  	{ /* sentinel */ }
>  };
>  
> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
> index 668e7358f19b..ec964c43c932 100644
> --- a/drivers/mtd/nand/atmel_nand_ecc.h
> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
> @@ -108,7 +108,11 @@
>  #define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
>  #define		PMERRLOC_CALC_DONE		(1 << 0)
>  #define ATMEL_PMERRLOC_SIGMAx		0x028	/* Error location SIGMA x */
> -#define ATMEL_PMERRLOC_ELx		0x08c	/* Error location x */
> +
> +/*
> + * The ATMEL_PMERRLOC_ELx register location depends from the number of
> + * bits corrected by the PMECC controller. Do not use it.
> + */
>  
>  /* Register access macros for PMECC */
>  #define pmecc_readl_relaxed(addr, reg) \
> @@ -136,7 +140,7 @@
>  	readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
>  
>  #define pmerrloc_readl_el_relaxed(addr, n) \
> -	readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
> +	readl_relaxed((addr) + ((n) * 4))

Another option would have been to pass the maximum ECC strength here,
and modify the ATMEL_PMERRLOC_ELx macro to take a max_strength argument
and return the adjusted ELx offset.
Anyway, both solutions work for me.

>  
>  /* Galois field dimension */
>  #define PMECC_GF_DIMENSION_13			13



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

Re: [PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:16 +0100
Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
> as a valid setting when required by the device tree or the NAND
> parameter page.
> 
> Then configure the controller to do use this new setting.
> 
> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-
>  drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 90887b430f03..ef0db8e2a0fd 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -27,7 +27,8 @@ Optional properties:
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>    Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
> -  Controller. Supported values are: 2, 4, 8, 12, 24.
> +  Controller. Supported values are: 2, 4, 8, 12, 24. SAMA5D2 also supports
> +  32.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
>    are: 512, 1024.
>  - atmel,pmecc-lookup-table-offset : includes two offsets of lookup table in ROM
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 6fe50e2d291f..920a0a315a60 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -474,6 +474,7 @@ static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
>   *                8-bits                13-bytes                 14-bytes
>   *               12-bits                20-bytes                 21-bytes
>   *               24-bits                39-bytes                 42-bytes
> + *               32-bits                52-bytes                 56-bytes
>   */
>  static int pmecc_get_ecc_bytes(int cap, int sector_size)
>  {
> @@ -1022,6 +1023,9 @@ static void atmel_pmecc_core_init(struct mtd_info *mtd)
>  	case 24:
>  		val = PMECC_CFG_BCH_ERR24;
>  		break;
> +	case 32:
> +		val = PMECC_CFG_BCH_ERR32;
> +		break;
>  	}
>  
>  	if (host->pmecc_sector_size == 512)
> @@ -1083,6 +1087,9 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  
>  	/* If device tree doesn't specify, use NAND's minimum ECC parameters */
>  	if (host->pmecc_corr_cap == 0) {
> +		if (*cap > host->caps->pmecc_max_correction)
> +			return -EINVAL;
> +
>  		/* use the most fitable ecc bits (the near bigger one ) */
>  		if (*cap <= 2)
>  			host->pmecc_corr_cap = 2;
> @@ -1094,6 +1101,8 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  			host->pmecc_corr_cap = 12;
>  		else if (*cap <= 24)
>  			host->pmecc_corr_cap = 24;
> +		else if (*cap <= 32)
> +			host->pmecc_corr_cap = 32;
>  		else
>  			return -EINVAL;
>  	}
> @@ -1554,10 +1563,16 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>  	 * them from NAND ONFI parameters.
>  	 */
>  	if (of_property_read_u32(np, "atmel,pmecc-cap", &val) == 0) {
> -		if ((val != 2) && (val != 4) && (val != 8) && (val != 12) &&
> -				(val != 24)) {
> +		if (val > host->caps->pmecc_max_correction) {
> +			dev_err(host->dev,
> +				"Required ECC strength too high: %u max %u\n",
> +				val, host->caps->pmecc_max_correction);
> +			return -EINVAL;
> +		}
> +		if ((val != 2)  && (val != 4)  && (val != 8) &&
> +		    (val != 12) && (val != 24) && (val != 32)) {
>  			dev_err(host->dev,
> -				"Unsupported PMECC correction capability: %d; should be 2, 4, 8, 12 or 24\n",
> +				"Required ECC strength not supported: %u\n",
>  				val);
>  			return -EINVAL;
>  		}
> @@ -1567,7 +1582,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>  	if (of_property_read_u32(np, "atmel,pmecc-sector-size", &val) == 0) {
>  		if ((val != 512) && (val != 1024)) {
>  			dev_err(host->dev,
> -				"Unsupported PMECC sector size: %d; should be 512 or 1024 bytes\n",
> +				"Required ECC sector size not supported: %u\n",
>  				val);

I'm nitpicking, but this change has nothing to do with the addition
of the 32bits strength. Maybe it should be part of another patch (along
with the other log message rewording).

>  			return -EINVAL;
>  		}
> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
> index ec964c43c932..834d694487bd 100644
> --- a/drivers/mtd/nand/atmel_nand_ecc.h
> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
> @@ -43,6 +43,7 @@
>  #define		PMECC_CFG_BCH_ERR8		(2 << 0)
>  #define		PMECC_CFG_BCH_ERR12		(3 << 0)
>  #define		PMECC_CFG_BCH_ERR24		(4 << 0)
> +#define		PMECC_CFG_BCH_ERR32		(5 << 0)
>  
>  #define		PMECC_CFG_SECTOR512		(0 << 4)
>  #define		PMECC_CFG_SECTOR1024		(1 << 4)



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength

[Boris Brezillon]

On Wed, 13 Jan 2016 17:34:16 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> As the SAMA5D2 controller supports the 32-bit ECC strength, accept it
> as a valid setting when required by the device tree or the NAND
> parameter page.
> 
> Then configure the controller to do use this new setting.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  3 ++-
>  drivers/mtd/nand/atmel_nand.c                      | 23 ++++++++++++++++++----
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 90887b430f03..ef0db8e2a0fd 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -27,7 +27,8 @@ Optional properties:
>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>    Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
> -  Controller. Supported values are: 2, 4, 8, 12, 24.
> +  Controller. Supported values are: 2, 4, 8, 12, 24. SAMA5D2 also supports
> +  32.
>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
>    are: 512, 1024.
>  - atmel,pmecc-lookup-table-offset : includes two offsets of lookup table in ROM
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 6fe50e2d291f..920a0a315a60 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -474,6 +474,7 @@ static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
>   *                8-bits                13-bytes                 14-bytes
>   *               12-bits                20-bytes                 21-bytes
>   *               24-bits                39-bytes                 42-bytes
> + *               32-bits                52-bytes                 56-bytes
>   */
>  static int pmecc_get_ecc_bytes(int cap, int sector_size)
>  {
> @@ -1022,6 +1023,9 @@ static void atmel_pmecc_core_init(struct mtd_info *mtd)
>  	case 24:
>  		val = PMECC_CFG_BCH_ERR24;
>  		break;
> +	case 32:
> +		val = PMECC_CFG_BCH_ERR32;
> +		break;
>  	}
>  
>  	if (host->pmecc_sector_size == 512)
> @@ -1083,6 +1087,9 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  
>  	/* If device tree doesn't specify, use NAND's minimum ECC parameters */
>  	if (host->pmecc_corr_cap == 0) {
> +		if (*cap > host->caps->pmecc_max_correction)
> +			return -EINVAL;
> +
>  		/* use the most fitable ecc bits (the near bigger one ) */
>  		if (*cap <= 2)
>  			host->pmecc_corr_cap = 2;
> @@ -1094,6 +1101,8 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  			host->pmecc_corr_cap = 12;
>  		else if (*cap <= 24)
>  			host->pmecc_corr_cap = 24;
> +		else if (*cap <= 32)
> +			host->pmecc_corr_cap = 32;
>  		else
>  			return -EINVAL;
>  	}
> @@ -1554,10 +1563,16 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>  	 * them from NAND ONFI parameters.
>  	 */
>  	if (of_property_read_u32(np, "atmel,pmecc-cap", &val) == 0) {
> -		if ((val != 2) && (val != 4) && (val != 8) && (val != 12) &&
> -				(val != 24)) {
> +		if (val > host->caps->pmecc_max_correction) {
> +			dev_err(host->dev,
> +				"Required ECC strength too high: %u max %u\n",
> +				val, host->caps->pmecc_max_correction);
> +			return -EINVAL;
> +		}
> +		if ((val != 2)  && (val != 4)  && (val != 8) &&
> +		    (val != 12) && (val != 24) && (val != 32)) {
>  			dev_err(host->dev,
> -				"Unsupported PMECC correction capability: %d; should be 2, 4, 8, 12 or 24\n",
> +				"Required ECC strength not supported: %u\n",
>  				val);
>  			return -EINVAL;
>  		}
> @@ -1567,7 +1582,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>  	if (of_property_read_u32(np, "atmel,pmecc-sector-size", &val) == 0) {
>  		if ((val != 512) && (val != 1024)) {
>  			dev_err(host->dev,
> -				"Unsupported PMECC sector size: %d; should be 512 or 1024 bytes\n",
> +				"Required ECC sector size not supported: %u\n",
>  				val);

I'm nitpicking, but this change has nothing to do with the addition
of the 32bits strength. Maybe it should be part of another patch (along
with the other log message rewording).

>  			return -EINVAL;
>  		}
> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
> index ec964c43c932..834d694487bd 100644
> --- a/drivers/mtd/nand/atmel_nand_ecc.h
> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
> @@ -43,6 +43,7 @@
>  #define		PMECC_CFG_BCH_ERR8		(2 << 0)
>  #define		PMECC_CFG_BCH_ERR12		(3 << 0)
>  #define		PMECC_CFG_BCH_ERR24		(4 << 0)
> +#define		PMECC_CFG_BCH_ERR32		(5 << 0)
>  
>  #define		PMECC_CFG_SECTOR512		(0 << 4)
>  #define		PMECC_CFG_SECTOR1024		(1 << 4)



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

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Romain Izard]

Hi Boris,

2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index 9d71f9e6a8de..e5d7e7e63f49 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
>>       bool pmecc_correct_erase_page;
>>  };
>>
>> +struct atmel_nand_nfc_priv {
>
> Can you rename this struct into atmel_nfc_caps to be consistant with the
> atmel_nand_caps?
>
>> +     uint32_t rb_edge;
>
> Actually, AFAIU, it's not encoding the type of edges, but the available
> native R/B lines (by native I mean the R/B lines directly connected to
> the NFC IP).
> I suggest renaming this field avail_rb_lines, and making it a bitfield
> (one bit per possible R/B line).
>

It's already a bitfield in the end: it's the mask for the interrupt status
register. It might have been better if it were called rb_edge_mask.

>> +};
>> +
>>  /* oob layout for large page size
>>   * bad block info is on bytes 0 and 1
>>   * the bytes have to be consecutives to avoid
>> @@ -111,6 +115,7 @@ struct atmel_nfc {
>>       /* Point to the sram bank which include readed data via NFC */
>>       void                    *data_in_sram;
>>       bool                    will_write_sram;
>> +     uint32_t                rb_edge;
>
> Replace this by
>         const struct atmel_nfc_caps *caps;
>
> so that next time you have a per-SoC particularity, you can just add it
> to your struct atmel_nfc_caps without adding new fields to atmel_nfc.
>

I'm reluctant about this, but I will do it. For me, we are exchanging
a single probe-time copy to indirections in each interrupt handler for some
unspecified future user. Let's hope the tradeoff is worth it.

>>  };
>>  static struct atmel_nfc      nand_nfc;
>>
>> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
>>               nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
>>               ret = IRQ_HANDLED;
>>       }
>> -     if (pending & NFC_SR_RB_EDGE) {
>> +     if (pending & host->nfc->rb_edge) {
>>               complete(&host->nfc->comp_ready);
>> -             nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
>> +             nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
>
> How about creating a macro that would generate the appropriate bitmask
> for you?
>
> Something like
>
> #define NFC_SR_RB_EDGE_MASK(x)  ((x) << 24)
>

It's already so verbose, it will make everything longer. Once the member name
contains the 'mask' part, all the information will be there.


>> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
>> index 4d5d26221a7e..2cd9c57b1e53 100644
>> --- a/drivers/mtd/nand/atmel_nand_nfc.h
>> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
>> @@ -42,7 +42,10 @@
>>  #define              NFC_SR_UNDEF            (1 << 21)
>>  #define              NFC_SR_AWB              (1 << 22)
>>  #define              NFC_SR_ASE              (1 << 23)
>> -#define              NFC_SR_RB_EDGE          (1 << 24)
>> +#define              NFC_SR_RB_EDGE0         (1 << 24)
>> +#define              NFC_SR_RB_EDGE1         (1 << 25)
>> +#define              NFC_SR_RB_EDGE2         (1 << 26)
>> +#define              NFC_SR_RB_EDGE3         (1 << 27)
>
> Please replace those 4 definitions by:
>
> #define                 NFC_SR_RB_EDGE(x)       BIT(x + 24)
>

The whole file could be rewritten in this form, but I see this as a
zero-value proposition. I believe it is best to keep with local
consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as
those are not mentioned in the datasheets.


Best regards,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Romain Izard]

Hi Boris,

2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index 9d71f9e6a8de..e5d7e7e63f49 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
>>       bool pmecc_correct_erase_page;
>>  };
>>
>> +struct atmel_nand_nfc_priv {
>
> Can you rename this struct into atmel_nfc_caps to be consistant with the
> atmel_nand_caps?
>
>> +     uint32_t rb_edge;
>
> Actually, AFAIU, it's not encoding the type of edges, but the available
> native R/B lines (by native I mean the R/B lines directly connected to
> the NFC IP).
> I suggest renaming this field avail_rb_lines, and making it a bitfield
> (one bit per possible R/B line).
>

It's already a bitfield in the end: it's the mask for the interrupt status
register. It might have been better if it were called rb_edge_mask.

>> +};
>> +
>>  /* oob layout for large page size
>>   * bad block info is on bytes 0 and 1
>>   * the bytes have to be consecutives to avoid
>> @@ -111,6 +115,7 @@ struct atmel_nfc {
>>       /* Point to the sram bank which include readed data via NFC */
>>       void                    *data_in_sram;
>>       bool                    will_write_sram;
>> +     uint32_t                rb_edge;
>
> Replace this by
>         const struct atmel_nfc_caps *caps;
>
> so that next time you have a per-SoC particularity, you can just add it
> to your struct atmel_nfc_caps without adding new fields to atmel_nfc.
>

I'm reluctant about this, but I will do it. For me, we are exchanging
a single probe-time copy to indirections in each interrupt handler for some
unspecified future user. Let's hope the tradeoff is worth it.

>>  };
>>  static struct atmel_nfc      nand_nfc;
>>
>> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
>>               nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
>>               ret = IRQ_HANDLED;
>>       }
>> -     if (pending & NFC_SR_RB_EDGE) {
>> +     if (pending & host->nfc->rb_edge) {
>>               complete(&host->nfc->comp_ready);
>> -             nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
>> +             nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
>
> How about creating a macro that would generate the appropriate bitmask
> for you?
>
> Something like
>
> #define NFC_SR_RB_EDGE_MASK(x)  ((x) << 24)
>

It's already so verbose, it will make everything longer. Once the member name
contains the 'mask' part, all the information will be there.


>> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
>> index 4d5d26221a7e..2cd9c57b1e53 100644
>> --- a/drivers/mtd/nand/atmel_nand_nfc.h
>> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
>> @@ -42,7 +42,10 @@
>>  #define              NFC_SR_UNDEF            (1 << 21)
>>  #define              NFC_SR_AWB              (1 << 22)
>>  #define              NFC_SR_ASE              (1 << 23)
>> -#define              NFC_SR_RB_EDGE          (1 << 24)
>> +#define              NFC_SR_RB_EDGE0         (1 << 24)
>> +#define              NFC_SR_RB_EDGE1         (1 << 25)
>> +#define              NFC_SR_RB_EDGE2         (1 << 26)
>> +#define              NFC_SR_RB_EDGE3         (1 << 27)
>
> Please replace those 4 definitions by:
>
> #define                 NFC_SR_RB_EDGE(x)       BIT(x + 24)
>

The whole file could be rewritten in this form, but I see this as a
zero-value proposition. I believe it is best to keep with local
consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as
those are not mentioned in the datasheets.


Best regards,

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Boris Brezillon]

On Thu, 14 Jan 2016 11:16:19 +0100
Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Hi Boris,
> 
> 2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> >> index 9d71f9e6a8de..e5d7e7e63f49 100644
> >> --- a/drivers/mtd/nand/atmel_nand.c
> >> +++ b/drivers/mtd/nand/atmel_nand.c
> >> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
> >>       bool pmecc_correct_erase_page;
> >>  };
> >>
> >> +struct atmel_nand_nfc_priv {
> >
> > Can you rename this struct into atmel_nfc_caps to be consistant with the
> > atmel_nand_caps?
> >
> >> +     uint32_t rb_edge;
> >
> > Actually, AFAIU, it's not encoding the type of edges, but the available
> > native R/B lines (by native I mean the R/B lines directly connected to
> > the NFC IP).
> > I suggest renaming this field avail_rb_lines, and making it a bitfield
> > (one bit per possible R/B line).
> >
> 
> It's already a bitfield in the end: it's the mask for the interrupt status
> register. It might have been better if it were called rb_edge_mask.

Actually the naming chosen by atmel is a bit misleading, cause the
R/B edge selection is done through the RB_RISE/FALL fields,
and what they call RB_EDGEX is just the R/B line selection, hence the
suggestion to name this field avail_rb_lines.

> 
> >> +};
> >> +
> >>  /* oob layout for large page size
> >>   * bad block info is on bytes 0 and 1
> >>   * the bytes have to be consecutives to avoid
> >> @@ -111,6 +115,7 @@ struct atmel_nfc {
> >>       /* Point to the sram bank which include readed data via NFC */
> >>       void                    *data_in_sram;
> >>       bool                    will_write_sram;
> >> +     uint32_t                rb_edge;
> >
> > Replace this by
> >         const struct atmel_nfc_caps *caps;
> >
> > so that next time you have a per-SoC particularity, you can just add it
> > to your struct atmel_nfc_caps without adding new fields to atmel_nfc.
> >
> 
> I'm reluctant about this, but I will do it. For me, we are exchanging
> a single probe-time copy to indirections in each interrupt handler for some
> unspecified future user. Let's hope the tradeoff is worth it.

You raised a good point, maybe it's worth keeping this rb_edge field
directly in the atmel_nfc struct. If you decide to do so, could you add
a comment explaining why you're doing it?

> 
> >>  };
> >>  static struct atmel_nfc      nand_nfc;
> >>
> >> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
> >>               nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
> >>               ret = IRQ_HANDLED;
> >>       }
> >> -     if (pending & NFC_SR_RB_EDGE) {
> >> +     if (pending & host->nfc->rb_edge) {

In the other hand, I see that we're already having a lot of
indirections here, not sure adding one more will drastically impact the
system.
And if you really care about interrupt latencies in the system, you
should probably register this handler as a threaded irq.

> >>               complete(&host->nfc->comp_ready);
> >> -             nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
> >> +             nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
> >
> > How about creating a macro that would generate the appropriate bitmask
> > for you?
> >
> > Something like
> >
> > #define NFC_SR_RB_EDGE_MASK(x)  ((x) << 24)
> >
> 
> It's already so verbose, it will make everything longer. Once the member name
> contains the 'mask' part, all the information will be there.

Makes sense, especially if you keep the pre-calculated nfc->rb_edge_mask
field.

> 
> 
> >> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
> >> index 4d5d26221a7e..2cd9c57b1e53 100644
> >> --- a/drivers/mtd/nand/atmel_nand_nfc.h
> >> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
> >> @@ -42,7 +42,10 @@
> >>  #define              NFC_SR_UNDEF            (1 << 21)
> >>  #define              NFC_SR_AWB              (1 << 22)
> >>  #define              NFC_SR_ASE              (1 << 23)
> >> -#define              NFC_SR_RB_EDGE          (1 << 24)
> >> +#define              NFC_SR_RB_EDGE0         (1 << 24)
> >> +#define              NFC_SR_RB_EDGE1         (1 << 25)
> >> +#define              NFC_SR_RB_EDGE2         (1 << 26)
> >> +#define              NFC_SR_RB_EDGE3         (1 << 27)
> >
> > Please replace those 4 definitions by:
> >
> > #define                 NFC_SR_RB_EDGE(x)       BIT(x + 24)
> >
> 
> The whole file could be rewritten in this form, but I see this as a
> zero-value proposition. I believe it is best to keep with local
> consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as
> those are not mentioned in the datasheets.

Hm, I keep thinking using the NFC_SR_RB_EDGE(x) is more future proof,
but I'll let Atmel maintainers decide whether it's relevant or not to
do so.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

[Boris Brezillon]

On Thu, 14 Jan 2016 11:16:19 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> Hi Boris,
> 
> 2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> >> index 9d71f9e6a8de..e5d7e7e63f49 100644
> >> --- a/drivers/mtd/nand/atmel_nand.c
> >> +++ b/drivers/mtd/nand/atmel_nand.c
> >> @@ -67,6 +67,10 @@ struct atmel_nand_caps {
> >>       bool pmecc_correct_erase_page;
> >>  };
> >>
> >> +struct atmel_nand_nfc_priv {
> >
> > Can you rename this struct into atmel_nfc_caps to be consistant with the
> > atmel_nand_caps?
> >
> >> +     uint32_t rb_edge;
> >
> > Actually, AFAIU, it's not encoding the type of edges, but the available
> > native R/B lines (by native I mean the R/B lines directly connected to
> > the NFC IP).
> > I suggest renaming this field avail_rb_lines, and making it a bitfield
> > (one bit per possible R/B line).
> >
> 
> It's already a bitfield in the end: it's the mask for the interrupt status
> register. It might have been better if it were called rb_edge_mask.

Actually the naming chosen by atmel is a bit misleading, cause the
R/B edge selection is done through the RB_RISE/FALL fields,
and what they call RB_EDGEX is just the R/B line selection, hence the
suggestion to name this field avail_rb_lines.

> 
> >> +};
> >> +
> >>  /* oob layout for large page size
> >>   * bad block info is on bytes 0 and 1
> >>   * the bytes have to be consecutives to avoid
> >> @@ -111,6 +115,7 @@ struct atmel_nfc {
> >>       /* Point to the sram bank which include readed data via NFC */
> >>       void                    *data_in_sram;
> >>       bool                    will_write_sram;
> >> +     uint32_t                rb_edge;
> >
> > Replace this by
> >         const struct atmel_nfc_caps *caps;
> >
> > so that next time you have a per-SoC particularity, you can just add it
> > to your struct atmel_nfc_caps without adding new fields to atmel_nfc.
> >
> 
> I'm reluctant about this, but I will do it. For me, we are exchanging
> a single probe-time copy to indirections in each interrupt handler for some
> unspecified future user. Let's hope the tradeoff is worth it.

You raised a good point, maybe it's worth keeping this rb_edge field
directly in the atmel_nfc struct. If you decide to do so, could you add
a comment explaining why you're doing it?

> 
> >>  };
> >>  static struct atmel_nfc      nand_nfc;
> >>
> >> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
> >>               nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
> >>               ret = IRQ_HANDLED;
> >>       }
> >> -     if (pending & NFC_SR_RB_EDGE) {
> >> +     if (pending & host->nfc->rb_edge) {

In the other hand, I see that we're already having a lot of
indirections here, not sure adding one more will drastically impact the
system.
And if you really care about interrupt latencies in the system, you
should probably register this handler as a threaded irq.

> >>               complete(&host->nfc->comp_ready);
> >> -             nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
> >> +             nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);
> >
> > How about creating a macro that would generate the appropriate bitmask
> > for you?
> >
> > Something like
> >
> > #define NFC_SR_RB_EDGE_MASK(x)  ((x) << 24)
> >
> 
> It's already so verbose, it will make everything longer. Once the member name
> contains the 'mask' part, all the information will be there.

Makes sense, especially if you keep the pre-calculated nfc->rb_edge_mask
field.

> 
> 
> >> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
> >> index 4d5d26221a7e..2cd9c57b1e53 100644
> >> --- a/drivers/mtd/nand/atmel_nand_nfc.h
> >> +++ b/drivers/mtd/nand/atmel_nand_nfc.h
> >> @@ -42,7 +42,10 @@
> >>  #define              NFC_SR_UNDEF            (1 << 21)
> >>  #define              NFC_SR_AWB              (1 << 22)
> >>  #define              NFC_SR_ASE              (1 << 23)
> >> -#define              NFC_SR_RB_EDGE          (1 << 24)
> >> +#define              NFC_SR_RB_EDGE0         (1 << 24)
> >> +#define              NFC_SR_RB_EDGE1         (1 << 25)
> >> +#define              NFC_SR_RB_EDGE2         (1 << 26)
> >> +#define              NFC_SR_RB_EDGE3         (1 << 27)
> >
> > Please replace those 4 definitions by:
> >
> > #define                 NFC_SR_RB_EDGE(x)       BIT(x + 24)
> >
> 
> The whole file could be rewritten in this form, but I see this as a
> zero-value proposition. I believe it is best to keep with local
> consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as
> those are not mentioned in the datasheets.

Hm, I keep thinking using the NFC_SR_RB_EDGE(x) is more future proof,
but I'll let Atmel maintainers decide whether it's relevant or not to
do so.


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

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>
>
>> -----Original Message-----
>> From: Rob Herring [mailto:robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
>> Sent: 2016年1月14日 9:13
>> To: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yang, Wenyou
>> <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>; Josh Wu <rainyfeeling-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>; Ferre,
>> Nicolas <Nicolas.FERRE-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
>>
>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>> > controller that can correct 32 bits in each sector. This controller is
>> > not 100% compatible with the previous revision that corrected a
>> > maximum of 24 bits by sector, as some register addresses overlap.
>> >
>> > Using information from the device tree, we can configure the driver to
>> > work with both versions.
>> >
>> > Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>> >  3 files changed, 33 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > index 89b0db9801b0..90887b430f03 100644
>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > @@ -1,7 +1,10 @@
>> >  Atmel NAND flash
>> >
>> >  Required properties:
>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>> > +- compatible: The possible values are:
>> > +   "atmel,at91rm9200-nand"
>> > +   "atmel,sama5d2-nand"
>> > +   "atmel,sama5d4-nand"
>> >  - reg : should specify localbus address and size used for the chip,
>> >     and hardware ECC controller if available.
>> >     If the hardware ECC is PMECC, it should contain address and size for
>> > @@ -22,7 +25,7 @@ Optional properties:
>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>> >    "soft_bch".
>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>> > -  Only supported by at91sam9x5 or later sam9 product.
>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>
>> What compatible string would AT91SAM9x5 be?
>
> "atmel,at91rm9200-nand".

Answer the question in the binding doc by saying which compatible
strings a property is valid for.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
>
>
>> -----Original Message-----
>> From: Rob Herring [mailto:robh@kernel.org]
>> Sent: 2016年1月14日 9:13
>> To: Romain Izard <romain.izard.pro@gmail.com>
>> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou
>> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre,
>> Nicolas <Nicolas.FERRE@atmel.com>
>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
>>
>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>> > controller that can correct 32 bits in each sector. This controller is
>> > not 100% compatible with the previous revision that corrected a
>> > maximum of 24 bits by sector, as some register addresses overlap.
>> >
>> > Using information from the device tree, we can configure the driver to
>> > work with both versions.
>> >
>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> > ---
>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>> >  3 files changed, 33 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > index 89b0db9801b0..90887b430f03 100644
>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> > @@ -1,7 +1,10 @@
>> >  Atmel NAND flash
>> >
>> >  Required properties:
>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>> > +- compatible: The possible values are:
>> > +   "atmel,at91rm9200-nand"
>> > +   "atmel,sama5d2-nand"
>> > +   "atmel,sama5d4-nand"
>> >  - reg : should specify localbus address and size used for the chip,
>> >     and hardware ECC controller if available.
>> >     If the hardware ECC is PMECC, it should contain address and size for
>> > @@ -22,7 +25,7 @@ Optional properties:
>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>> >    "soft_bch".
>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>> > -  Only supported by at91sam9x5 or later sam9 product.
>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>
>> What compatible string would AT91SAM9x5 be?
>
> "atmel,at91rm9200-nand".

Answer the question in the binding doc by saying which compatible
strings a property is valid for.

Rob

RE: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Yang, Wenyou]

Hi Rob,

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2016年1月14日 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Romain Izard <romain.izard.pro@gmail.com>; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Josh Wu <rainyfeeling@outlook.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>
> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> 
> On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Rob Herring [mailto:robh@kernel.org]
> >> Sent: 2016年1月14日 9:13
> >> To: Romain Izard <romain.izard.pro@gmail.com>
> >> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang,
> >> Wenyou <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>;
> >> Ferre, Nicolas <Nicolas.FERRE@atmel.com>
> >> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> >>
> >> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> >> > Starting with the SAMA5D2, there is a new revision of the Atmel
> >> > PMECC controller that can correct 32 bits in each sector. This
> >> > controller is not 100% compatible with the previous revision that
> >> > corrected a maximum of 24 bits by sector, as some register addresses
> overlap.
> >> >
> >> > Using information from the device tree, we can configure the driver
> >> > to work with both versions.
> >> >
> >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> >> > ---
> >> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >> >  drivers/mtd/nand/atmel_nand.c                      | 23
> +++++++++++++++++++++-
> >> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > index 89b0db9801b0..90887b430f03 100644
> >> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > @@ -1,7 +1,10 @@
> >> >  Atmel NAND flash
> >> >
> >> >  Required properties:
> >> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> >> > +- compatible: The possible values are:
> >> > +   "atmel,at91rm9200-nand"
> >> > +   "atmel,sama5d2-nand"
> >> > +   "atmel,sama5d4-nand"
> >> >  - reg : should specify localbus address and size used for the chip,
> >> >     and hardware ECC controller if available.
> >> >     If the hardware ECC is PMECC, it should contain address and
> >> > size for @@ -22,7 +25,7 @@ Optional properties:
> >> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >> >    "soft_bch".
> >> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC
> hardware.
> >> > -  Only supported by at91sam9x5 or later sam9 product.
> >> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> >>
> >> What compatible string would AT91SAM9x5 be?
> >
> > "atmel,at91rm9200-nand".
> 
> Answer the question in the binding doc by saying which compatible strings a
> property is valid for.
> 

Thank you for your advice. I will keep in mind next time.


Best Regards,
Wenyou Yang

RE: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Yang, Wenyou]

Hi Rob,

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2016年1月14日 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Romain Izard <romain.izard.pro@gmail.com>; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Josh Wu <rainyfeeling@outlook.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>
> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> 
> On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Rob Herring [mailto:robh@kernel.org]
> >> Sent: 2016年1月14日 9:13
> >> To: Romain Izard <romain.izard.pro@gmail.com>
> >> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang,
> >> Wenyou <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>;
> >> Ferre, Nicolas <Nicolas.FERRE@atmel.com>
> >> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> >>
> >> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> >> > Starting with the SAMA5D2, there is a new revision of the Atmel
> >> > PMECC controller that can correct 32 bits in each sector. This
> >> > controller is not 100% compatible with the previous revision that
> >> > corrected a maximum of 24 bits by sector, as some register addresses
> overlap.
> >> >
> >> > Using information from the device tree, we can configure the driver
> >> > to work with both versions.
> >> >
> >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> >> > ---
> >> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >> >  drivers/mtd/nand/atmel_nand.c                      | 23
> +++++++++++++++++++++-
> >> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > index 89b0db9801b0..90887b430f03 100644
> >> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >> > @@ -1,7 +1,10 @@
> >> >  Atmel NAND flash
> >> >
> >> >  Required properties:
> >> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> >> > +- compatible: The possible values are:
> >> > +   "atmel,at91rm9200-nand"
> >> > +   "atmel,sama5d2-nand"
> >> > +   "atmel,sama5d4-nand"
> >> >  - reg : should specify localbus address and size used for the chip,
> >> >     and hardware ECC controller if available.
> >> >     If the hardware ECC is PMECC, it should contain address and
> >> > size for @@ -22,7 +25,7 @@ Optional properties:
> >> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >> >    "soft_bch".
> >> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC
> hardware.
> >> > -  Only supported by at91sam9x5 or later sam9 product.
> >> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> >>
> >> What compatible string would AT91SAM9x5 be?
> >
> > "atmel,at91rm9200-nand".
> 
> Answer the question in the binding doc by saying which compatible strings a
> property is valid for.
> 

Thank you for your advice. I will keep in mind next time.


Best Regards,
Wenyou Yang

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Romain Izard]

Hi Rob, Wenyou,

2016-01-14 14:14 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Rob Herring [mailto:robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
>>> Sent: 2016年1月14日 9:13
>>> To: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yang, Wenyou
>>> <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>; Josh Wu <rainyfeeling-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>; Ferre,
>>> Nicolas <Nicolas.FERRE-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
>>>
>>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
>>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>>> > controller that can correct 32 bits in each sector. This controller is
>>> > not 100% compatible with the previous revision that corrected a
>>> > maximum of 24 bits by sector, as some register addresses overlap.
>>> >
>>> > Using information from the device tree, we can configure the driver to
>>> > work with both versions.
>>> >
>>> > Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> > ---
>>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>>> >  3 files changed, 33 insertions(+), 5 deletions(-)
>>> >
>>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > index 89b0db9801b0..90887b430f03 100644
>>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > @@ -1,7 +1,10 @@
>>> >  Atmel NAND flash
>>> >
>>> >  Required properties:
>>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>>> > +- compatible: The possible values are:
>>> > +   "atmel,at91rm9200-nand"
>>> > +   "atmel,sama5d2-nand"
>>> > +   "atmel,sama5d4-nand"
>>> >  - reg : should specify localbus address and size used for the chip,
>>> >     and hardware ECC controller if available.
>>> >     If the hardware ECC is PMECC, it should contain address and size for
>>> > @@ -22,7 +25,7 @@ Optional properties:
>>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>>> >    "soft_bch".
>>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>>> > -  Only supported by at91sam9x5 or later sam9 product.
>>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>>
>>> What compatible string would AT91SAM9x5 be?
>>
>> "atmel,at91rm9200-nand".
>
> Answer the question in the binding doc by saying which compatible
> strings a property is valid for.

I'm trying to rewrite the documentation for this point, and in the end it
seems to me that the line should be removed instead.

Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way
to describe that it supports this type of controller. If we need to write it
in the documentation as well, it's a duplicate information that can become
stale quite fast, as it was before.

In the end, I believe the best path is to describe better what PMECC is,
instead of describing as "the controller found in this kind of chip".

Best regards,
-- 
Romain Izard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Romain Izard]

Hi Rob, Wenyou,

2016-01-14 14:14 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Rob Herring [mailto:robh@kernel.org]
>>> Sent: 2016年1月14日 9:13
>>> To: Romain Izard <romain.izard.pro@gmail.com>
>>> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou
>>> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre,
>>> Nicolas <Nicolas.FERRE@atmel.com>
>>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
>>>
>>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
>>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>>> > controller that can correct 32 bits in each sector. This controller is
>>> > not 100% compatible with the previous revision that corrected a
>>> > maximum of 24 bits by sector, as some register addresses overlap.
>>> >
>>> > Using information from the device tree, we can configure the driver to
>>> > work with both versions.
>>> >
>>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>> > ---
>>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>>> >  3 files changed, 33 insertions(+), 5 deletions(-)
>>> >
>>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > index 89b0db9801b0..90887b430f03 100644
>>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>>> > @@ -1,7 +1,10 @@
>>> >  Atmel NAND flash
>>> >
>>> >  Required properties:
>>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>>> > +- compatible: The possible values are:
>>> > +   "atmel,at91rm9200-nand"
>>> > +   "atmel,sama5d2-nand"
>>> > +   "atmel,sama5d4-nand"
>>> >  - reg : should specify localbus address and size used for the chip,
>>> >     and hardware ECC controller if available.
>>> >     If the hardware ECC is PMECC, it should contain address and size for
>>> > @@ -22,7 +25,7 @@ Optional properties:
>>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>>> >    "soft_bch".
>>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>>> > -  Only supported by at91sam9x5 or later sam9 product.
>>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>>
>>> What compatible string would AT91SAM9x5 be?
>>
>> "atmel,at91rm9200-nand".
>
> Answer the question in the binding doc by saying which compatible
> strings a property is valid for.

I'm trying to rewrite the documentation for this point, and in the end it
seems to me that the line should be removed instead.

Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way
to describe that it supports this type of controller. If we need to write it
in the documentation as well, it's a duplicate information that can become
stale quite fast, as it was before.

In the end, I believe the best path is to describe better what PMECC is,
instead of describing as "the controller found in this kind of chip".

Best regards,
-- 
Romain Izard

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[romain izard]

Hi Boris,

2016-01-14 10:19 GMT+01:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Wed, 13 Jan 2016 17:34:15 +0100
> Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>> controller that can correct 32 bits in each sector. This controller is
>> not 100% compatible with the previous revision that corrected a maximum
>> of 24 bits by sector, as some register addresses overlap.
>>
>> Using information from the device tree, we can configure the driver to
>> work with both versions.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>>  3 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> index 89b0db9801b0..90887b430f03 100644
>> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> @@ -1,7 +1,10 @@
>>  Atmel NAND flash
>>
>>  Required properties:
>> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>> +- compatible: The possible values are:
>> +     "atmel,at91rm9200-nand"
>> +     "atmel,sama5d2-nand"
>> +     "atmel,sama5d4-nand"
>>  - reg : should specify localbus address and size used for the chip,
>>       and hardware ECC controller if available.
>>       If the hardware ECC is PMECC, it should contain address and size for
>> @@ -22,7 +25,7 @@ Optional properties:
>>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>>    "soft_bch".
>>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>> -  Only supported by at91sam9x5 or later sam9 product.
>> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>>    Controller. Supported values are: 2, 4, 8, 12, 24.
>>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index e5d7e7e63f49..6fe50e2d291f 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
>>
>>  struct atmel_nand_caps {
>>       bool pmecc_correct_erase_page;
>> +     uint8_t pmecc_max_correction;
>>  };
>>
>>  struct atmel_nand_nfc_priv {
>> @@ -146,6 +147,7 @@ struct atmel_nand_host {
>>       int                     pmecc_cw_len;   /* Length of codeword */
>>
>>       void __iomem            *pmerrloc_base;
>> +     void __iomem            *pmerrloc_el_base;
>>       void __iomem            *pmecc_rom_base;
>>
>>       /* lookup table for alpha_to and index_of */
>> @@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>>       sector_size = host->pmecc_sector_size;
>>
>>       while (err_nbr) {
>> -             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
>> +             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
>>               byte_pos = tmp / 8;
>>               bit_pos  = tmp % 8;
>>
>> @@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>>               err_no = PTR_ERR(host->pmerrloc_base);
>>               goto err;
>>       }
>
> How about putting the comment you added below here:
>
> /*
>  * The ATMEL_PMERRLOC_ELx register location depends on the number of
>  * bits corrected by the PMECC controller. Pre-calculate the
>  * pmerrloc_el_base according to the PMECC capabilities.
>  */
>
Here, this comment is paraphrasing the code. That's not very useful.

>> +     host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
>> +             (host->caps->pmecc_max_correction + 1) * 4;
>>
>>       if (!host->has_no_lookup_table) {
>>               regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
>> @@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +/*
>> + * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
>> + * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
>> + * devices from the SAM9 family that have those.
>> + */
>>  static struct atmel_nand_caps at91rm9200_caps = {
>>       .pmecc_correct_erase_page = false,
>> +     .pmecc_max_correction = 24,
>>  };
>>
>>  static struct atmel_nand_caps sama5d4_caps = {
>>       .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 24,
>> +};
>> +
>> +/*
>> + * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
>> + * as the increased correction strength requires more registers.
>> + */
>> +static struct atmel_nand_caps sama5d2_caps = {
>> +     .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 32,
>>  };
>>
>>  static const struct of_device_id atmel_nand_dt_ids[] = {
>>       { .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
>>       { .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
>> +     { .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
>>       { /* sentinel */ }
>>  };
>>
>> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
>> index 668e7358f19b..ec964c43c932 100644
>> --- a/drivers/mtd/nand/atmel_nand_ecc.h
>> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
>> @@ -108,7 +108,11 @@
>>  #define              PMERRLOC_ERR_NUM_MASK           (0x1f << 8)
>>  #define              PMERRLOC_CALC_DONE              (1 << 0)
>>  #define ATMEL_PMERRLOC_SIGMAx                0x028   /* Error location SIGMA x */
>> -#define ATMEL_PMERRLOC_ELx           0x08c   /* Error location x */
>> +
>> +/*
>> + * The ATMEL_PMERRLOC_ELx register location depends from the number of
>> + * bits corrected by the PMECC controller. Do not use it.
>> + */
>>
>>  /* Register access macros for PMECC */
>>  #define pmecc_readl_relaxed(addr, reg) \
>> @@ -136,7 +140,7 @@
>>       readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
>>
>>  #define pmerrloc_readl_el_relaxed(addr, n) \
>> -     readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
>> +     readl_relaxed((addr) + ((n) * 4))
>
> Another option would have been to pass the maximum ECC strength here,
> and modify the ATMEL_PMERRLOC_ELx macro to take a max_strength argument
> and return the adjusted ELx offset.
> Anyway, both solutions work for me.
>
The current solution makes it easier to keep within the 80 character limit.
The memory/computing tradeoff is so small it won't change anything.

>>
>>  /* Galois field dimension */
>>  #define PMECC_GF_DIMENSION_13                        13

Best regards,
-- 
Romain Izard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[romain izard]

Hi Boris,

2016-01-14 10:19 GMT+01:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 13 Jan 2016 17:34:15 +0100
> Romain Izard <romain.izard.pro@gmail.com> wrote:
>
>> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>> controller that can correct 32 bits in each sector. This controller is
>> not 100% compatible with the previous revision that corrected a maximum
>> of 24 bits by sector, as some register addresses overlap.
>>
>> Using information from the device tree, we can configure the driver to
>> work with both versions.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> ---
>>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>>  3 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> index 89b0db9801b0..90887b430f03 100644
>> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> @@ -1,7 +1,10 @@
>>  Atmel NAND flash
>>
>>  Required properties:
>> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>> +- compatible: The possible values are:
>> +     "atmel,at91rm9200-nand"
>> +     "atmel,sama5d2-nand"
>> +     "atmel,sama5d4-nand"
>>  - reg : should specify localbus address and size used for the chip,
>>       and hardware ECC controller if available.
>>       If the hardware ECC is PMECC, it should contain address and size for
>> @@ -22,7 +25,7 @@ Optional properties:
>>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>>    "soft_bch".
>>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>> -  Only supported by at91sam9x5 or later sam9 product.
>> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>>    Controller. Supported values are: 2, 4, 8, 12, 24.
>>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index e5d7e7e63f49..6fe50e2d291f 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
>>
>>  struct atmel_nand_caps {
>>       bool pmecc_correct_erase_page;
>> +     uint8_t pmecc_max_correction;
>>  };
>>
>>  struct atmel_nand_nfc_priv {
>> @@ -146,6 +147,7 @@ struct atmel_nand_host {
>>       int                     pmecc_cw_len;   /* Length of codeword */
>>
>>       void __iomem            *pmerrloc_base;
>> +     void __iomem            *pmerrloc_el_base;
>>       void __iomem            *pmecc_rom_base;
>>
>>       /* lookup table for alpha_to and index_of */
>> @@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>>       sector_size = host->pmecc_sector_size;
>>
>>       while (err_nbr) {
>> -             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
>> +             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
>>               byte_pos = tmp / 8;
>>               bit_pos  = tmp % 8;
>>
>> @@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>>               err_no = PTR_ERR(host->pmerrloc_base);
>>               goto err;
>>       }
>
> How about putting the comment you added below here:
>
> /*
>  * The ATMEL_PMERRLOC_ELx register location depends on the number of
>  * bits corrected by the PMECC controller. Pre-calculate the
>  * pmerrloc_el_base according to the PMECC capabilities.
>  */
>
Here, this comment is paraphrasing the code. That's not very useful.

>> +     host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
>> +             (host->caps->pmecc_max_correction + 1) * 4;
>>
>>       if (!host->has_no_lookup_table) {
>>               regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
>> @@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +/*
>> + * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
>> + * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
>> + * devices from the SAM9 family that have those.
>> + */
>>  static struct atmel_nand_caps at91rm9200_caps = {
>>       .pmecc_correct_erase_page = false,
>> +     .pmecc_max_correction = 24,
>>  };
>>
>>  static struct atmel_nand_caps sama5d4_caps = {
>>       .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 24,
>> +};
>> +
>> +/*
>> + * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
>> + * as the increased correction strength requires more registers.
>> + */
>> +static struct atmel_nand_caps sama5d2_caps = {
>> +     .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 32,
>>  };
>>
>>  static const struct of_device_id atmel_nand_dt_ids[] = {
>>       { .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
>>       { .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
>> +     { .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
>>       { /* sentinel */ }
>>  };
>>
>> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
>> index 668e7358f19b..ec964c43c932 100644
>> --- a/drivers/mtd/nand/atmel_nand_ecc.h
>> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
>> @@ -108,7 +108,11 @@
>>  #define              PMERRLOC_ERR_NUM_MASK           (0x1f << 8)
>>  #define              PMERRLOC_CALC_DONE              (1 << 0)
>>  #define ATMEL_PMERRLOC_SIGMAx                0x028   /* Error location SIGMA x */
>> -#define ATMEL_PMERRLOC_ELx           0x08c   /* Error location x */
>> +
>> +/*
>> + * The ATMEL_PMERRLOC_ELx register location depends from the number of
>> + * bits corrected by the PMECC controller. Do not use it.
>> + */
>>
>>  /* Register access macros for PMECC */
>>  #define pmecc_readl_relaxed(addr, reg) \
>> @@ -136,7 +140,7 @@
>>       readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
>>
>>  #define pmerrloc_readl_el_relaxed(addr, n) \
>> -     readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
>> +     readl_relaxed((addr) + ((n) * 4))
>
> Another option would have been to pass the maximum ECC strength here,
> and modify the ATMEL_PMERRLOC_ELx macro to take a max_strength argument
> and return the adjusted ELx offset.
> Anyway, both solutions work for me.
>
The current solution makes it easier to keep within the 80 character limit.
The memory/computing tradeoff is so small it won't change anything.

>>
>>  /* Galois field dimension */
>>  #define PMECC_GF_DIMENSION_13                        13

Best regards,
-- 
Romain Izard

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Fri, Jan 15, 2016 at 09:54:07AM +0100, Romain Izard wrote:
> Hi Rob, Wenyou,
> 
> 2016-01-14 14:14 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> > On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang-AIFe0yeh4nA@public.gmane.orgm> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Rob Herring [mailto:robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> >>> Sent: 2016年1月14日 9:13
> >>> To: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yang, Wenyou
> >>> <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>; Josh Wu <rainyfeeling-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>; Ferre,
> >>> Nicolas <Nicolas.FERRE-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> >>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> >>>
> >>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> >>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> >>> > controller that can correct 32 bits in each sector. This controller is
> >>> > not 100% compatible with the previous revision that corrected a
> >>> > maximum of 24 bits by sector, as some register addresses overlap.
> >>> >
> >>> > Using information from the device tree, we can configure the driver to
> >>> > work with both versions.
> >>> >
> >>> > Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> > ---
> >>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
> >>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >>> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >>> >
> >>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > index 89b0db9801b0..90887b430f03 100644
> >>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > @@ -1,7 +1,10 @@
> >>> >  Atmel NAND flash
> >>> >
> >>> >  Required properties:
> >>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> >>> > +- compatible: The possible values are:
> >>> > +   "atmel,at91rm9200-nand"
> >>> > +   "atmel,sama5d2-nand"
> >>> > +   "atmel,sama5d4-nand"
> >>> >  - reg : should specify localbus address and size used for the chip,
> >>> >     and hardware ECC controller if available.
> >>> >     If the hardware ECC is PMECC, it should contain address and size for
> >>> > @@ -22,7 +25,7 @@ Optional properties:
> >>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >>> >    "soft_bch".
> >>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> >>> > -  Only supported by at91sam9x5 or later sam9 product.
> >>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> >>>
> >>> What compatible string would AT91SAM9x5 be?
> >>
> >> "atmel,at91rm9200-nand".
> >
> > Answer the question in the binding doc by saying which compatible
> > strings a property is valid for.
> 
> I'm trying to rewrite the documentation for this point, and in the end it
> seems to me that the line should be removed instead.
> 
> Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way
> to describe that it supports this type of controller. If we need to write it
> in the documentation as well, it's a duplicate information that can become
> stale quite fast, as it was before.
> 
> In the end, I believe the best path is to describe better what PMECC is,
> instead of describing as "the controller found in this kind of chip".

Okay, that is important too. We are wanting to move binding 
documentation to something more structured where we can do some amount 
of validation of dts files against binding documentation. So that is why 
I am thinking in terms of having the information to validate a dts. But 
we are a ways off from doing such a thing.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Rob Herring]

On Fri, Jan 15, 2016 at 09:54:07AM +0100, Romain Izard wrote:
> Hi Rob, Wenyou,
> 
> 2016-01-14 14:14 GMT+01:00 Rob Herring <robh@kernel.org>:
> > On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Rob Herring [mailto:robh@kernel.org]
> >>> Sent: 2016年1月14日 9:13
> >>> To: Romain Izard <romain.izard.pro@gmail.com>
> >>> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou
> >>> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre,
> >>> Nicolas <Nicolas.FERRE@atmel.com>
> >>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2
> >>>
> >>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard wrote:
> >>> > Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
> >>> > controller that can correct 32 bits in each sector. This controller is
> >>> > not 100% compatible with the previous revision that corrected a
> >>> > maximum of 24 bits by sector, as some register addresses overlap.
> >>> >
> >>> > Using information from the device tree, we can configure the driver to
> >>> > work with both versions.
> >>> >
> >>> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> >>> > ---
> >>> >  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
> >>> >  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
> >>> >  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
> >>> >  3 files changed, 33 insertions(+), 5 deletions(-)
> >>> >
> >>> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > index 89b0db9801b0..90887b430f03 100644
> >>> > --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> >>> > @@ -1,7 +1,10 @@
> >>> >  Atmel NAND flash
> >>> >
> >>> >  Required properties:
> >>> > -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
> >>> > +- compatible: The possible values are:
> >>> > +   "atmel,at91rm9200-nand"
> >>> > +   "atmel,sama5d2-nand"
> >>> > +   "atmel,sama5d4-nand"
> >>> >  - reg : should specify localbus address and size used for the chip,
> >>> >     and hardware ECC controller if available.
> >>> >     If the hardware ECC is PMECC, it should contain address and size for
> >>> > @@ -22,7 +25,7 @@ Optional properties:
> >>> >    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >>> >    "soft_bch".
> >>> >  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
> >>> > -  Only supported by at91sam9x5 or later sam9 product.
> >>> > +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
> >>>
> >>> What compatible string would AT91SAM9x5 be?
> >>
> >> "atmel,at91rm9200-nand".
> >
> > Answer the question in the binding doc by saying which compatible
> > strings a property is valid for.
> 
> I'm trying to rewrite the documentation for this point, and in the end it
> seems to me that the line should be removed instead.
> 
> Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way
> to describe that it supports this type of controller. If we need to write it
> in the documentation as well, it's a duplicate information that can become
> stale quite fast, as it was before.
> 
> In the end, I believe the best path is to describe better what PMECC is,
> instead of describing as "the controller found in this kind of chip".

Okay, that is important too. We are wanting to move binding 
documentation to something more structured where we can do some amount 
of validation of dts files against binding documentation. So that is why 
I am thinking in terms of having the information to validate a dts. But 
we are a ways off from doing such a thing.

Rob

Re: [PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Brian Norris]

On Wed, Jan 13, 2016 at 06:54:21PM +0100, Boris Brezillon wrote:
> On Wed, 13 Jan 2016 17:34:13 +0100
> Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > When using multi-bit ECC, it is normal for the NAND Flash driver to
> > correct bit errors during the life of the product. Those errors will
> > only be cleared once a threshold has been reached, and corrections can
> > occur regularly before this.
> > 
> > Use only dev_dbg and not dev_info to report the bitflips, to keep the
> > system log clean when everything works correctly.
> > 
> > Signed-off-by: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Acked-by:  Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Pushed patch 1 to l2-mtd.git/next
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips

[Brian Norris]

On Wed, Jan 13, 2016 at 06:54:21PM +0100, Boris Brezillon wrote:
> On Wed, 13 Jan 2016 17:34:13 +0100
> Romain Izard <romain.izard.pro@gmail.com> wrote:
> 
> > When using multi-bit ECC, it is normal for the NAND Flash driver to
> > correct bit errors during the life of the product. Those errors will
> > only be cleared once a threshold has been reached, and corrections can
> > occur regularly before this.
> > 
> > Use only dev_dbg and not dev_info to report the bitflips, to keep the
> > system log clean when everything works correctly.
> > 
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > Acked-by:  Wenyou Yang <wenyou.yang@atmel.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Pushed patch 1 to l2-mtd.git/next