[RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

It took a couple months to update this series, but finally here is v2.
(v1: https://lkml.org/lkml/2016/11/26/144 )

This driver includes many problems.

One of the biggest one is a bunch of hard-coded parameters.  This IP
has many parameters that can be customized when a delivery RTL is
generated.  However, this driver was upstreamed by Intel, with
Intel parameters hard-coded.  Later, Altera added denali_dt.c to use
this driver for embedded boards, but they did not fix the code in
denali.c  So, this driver has never worked.  Even some DT bindings
actually turned out wrong.

There are more problems: [1] The driver just retrieves the OOB area as-is
whereas the controller uses syndrome page layout. [2] Many NAND chip
specific parameters are hard-coded in the driver. [3] ONFi devices are
not working  [4] It can not read Bad Block Marker

This patch series intends to solve those problems.

Since v1, here are more fixes/improvements:
 - Fix raw/oob callbacks for syndrome page layout
 - Implement setup_data_interface() callback
 - Fix/implement more commands for ONFi devices
 - Allow to skip the driver internal bounce buffer
 - Support PIO in case DMA is not supported
 - Switch from ->cmdfunc over to ->cmd_ctrl


Masahiro Yamada (53):
  mtd: nand: allow to set only one of ECC size and ECC strength from DT
  mtd: nand: use read_oob() instead of cmdfunc() for bad block check
  mtd: nand: denali: remove unused CONFIG option and macros
  mtd: nand: denali: remove redundant define of BANK(x)
  mtd: nand: denali: remove more unused struct members
  mtd: nand: denali: fix comment of denali_nand_info::flash_mem
  mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
  mtd: nand: denali: introduce capability flag
  mtd: nand: denali: use int where no reason to use fixed width variable
  mtd: nand: denali: fix erased page checking
  mtd: nand: denali: fix bitflips calculation in handle_ecc()
  mtd: nand: denali: support HW_ECC_FIXUP capability
  mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
  mtd: nand: denali: support 64bit capable DMA engine
  mtd: nand: denali_dt: remove dma-mask DT property
  mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
  mtd: nand: denali: allow to override revision number
  mtd: nand: denali: use nand_chip to hold frequently accessed data
  mtd: nand: denali: call nand_set_flash_node() to set DT node
  mtd: nand: denali: do not set mtd->name
  mtd: nand: denali: move multi device fixup code to a helper function
  mtd: nand: denali: simplify multi device fixup code
  mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
  mtd: nand: denali: remove meaningless writes to read-only registers
  mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
  mtd: nand: denali: support 1024 byte ECC step size
  mtd: nand: denali: avoid hard-coding ecc.strength and ecc.bytes
  mtd: nand: denali: support "nand-ecc-strength" DT property
  mtd: nand: denali: remove Toshiba and Hynix specific fixup code
  mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
  mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
  mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
  mtd: nand: denali: use BIT() and GENMASK() for register macros
  mtd: nand: denali: remove unneeded find_valid_banks()
  mtd: nand: denali: handle timing parameters by setup_data_interface()
  mtd: nand: denali: remove meaningless pipeline read-ahead operation
  mtd: nand: denali: rework interrupt handling
  mtd: nand: denali: fix NAND_CMD_STATUS handling
  mtd: nand: denali: fix NAND_CMD_PARAM handling
  mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
  mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)
  mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
  mtd: nand: denali: fix bank reset function
  mtd: nand: denali: use interrupt instead of polling for bank reset
  mtd: nand: denali: propagate page to helpers via function argument
  mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
  mtd: nand: denali: use flag instead of register macro for direction
  mtd: nand: denali: fix raw and oob accessors for syndrome page layout
  mtd: nand: denali: support hardware-assisted erased page detection
  mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
  mtd: nand: denali: skip driver internal bounce buffer when possible
  mtd: nand: denali: use non-managed kmalloc() for DMA buffer
  mtd: nand: denali: enable bad block table scan

 .../devicetree/bindings/mtd/denali-nand.txt        |   24 +-
 drivers/mtd/nand/Kconfig                           |   11 -
 drivers/mtd/nand/denali.c                          | 2073 +++++++++-----------
 drivers/mtd/nand/denali.h                          |  453 ++---
 drivers/mtd/nand/denali_dt.c                       |  104 +-
 drivers/mtd/nand/denali_pci.c                      |   10 +-
 drivers/mtd/nand/nand_base.c                       |   78 +-
 7 files changed, 1278 insertions(+), 1475 deletions(-)

-- 
2.7.4

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

[RESEND PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant

[Masahiro Yamada]

There are various customizable parameters, so several variants for
this IP.  A generic compatible like "denali,denali-nand-dt" is
useless.  Moreover, there are multiple things wrong with this string.
(Refer to Rob's comment [1])

The denali_dt.c was split out and this compatible was added by Altera
for the SOCFPGA port.  So, replace it with a more specific string.

The Denali IP on SOCFPGA incorporates the hardware ECC fixup feature.
So, this capability should be associated with the compatible string.

[1] https://lkml.org/lkml/2016/12/1/450

Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
---

Changes in v2:
  - Add caps flag to of_device_id data, not hard-code in driver code
  - Add Altera-specific compatible.

 Documentation/devicetree/bindings/mtd/denali-nand.txt |  5 +++--
 drivers/mtd/nand/denali_dt.c                          | 14 ++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index b04d03a..6f4ab4c 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -1,7 +1,8 @@
 * Denali NAND controller
 
 Required properties:
-  - compatible : should be "denali,denali-nand-dt"
+  - compatible : should be one of the following:
+      "altr,socfpga-denali-nand"            - for Altera SOCFPGA
   - reg : should contain registers location and length for data and reg.
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
@@ -15,7 +16,7 @@ Examples:
 nand: nand@ff900000 {
 	#address-cells = <1>;
 	#size-cells = <1>;
-	compatible = "denali,denali-nand-dt";
+	compatible = "altr,socfpga-denali-nand";
 	reg = <0xff900000 0x100000>, <0xffb80000 0x10000>;
 	reg-names = "nand_data", "denali_reg";
 	interrupts = <0 144 4>;
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 293ddb8..9577bfd 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -33,11 +33,17 @@ struct denali_dt_data {
 	unsigned int caps;
 };
 
-static const struct of_device_id denali_nand_dt_ids[] = {
-		{ .compatible = "denali,denali-nand-dt" },
-		{ /* sentinel */ }
-	};
+static const struct denali_dt_data denali_socfpga_data = {
+	.caps = DENALI_CAP_HW_ECC_FIXUP,
+};
 
+static const struct of_device_id denali_nand_dt_ids[] = {
+	{
+		.compatible = "altr,socfpga-denali-nand",
+		.data = &denali_socfpga_data,
+	},
+	{ /* sentinel */ }
+};
 MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
 
 static u64 denali_dma_mask;
-- 
2.7.4

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

[RESEND PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property

[Masahiro Yamada]

The driver sets appropriate DMA mask.  Delete the "dma-mask" DT
property.  See [1] for negative comments for this binding.

[1] https://lkml.org/lkml/2016/2/8/57

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes in v2: None

 Documentation/devicetree/bindings/mtd/denali-nand.txt | 2 --
 drivers/mtd/nand/denali_dt.c                          | 9 ---------
 2 files changed, 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 6f4ab4c..e593bbe 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -6,7 +6,6 @@ Required properties:
   - reg : should contain registers location and length for data and reg.
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
-  - dm-mask : DMA bit mask
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
@@ -20,5 +19,4 @@ nand: nand@ff900000 {
 	reg = <0xff900000 0x100000>, <0xffb80000 0x10000>;
 	reg-names = "nand_data", "denali_reg";
 	interrupts = <0 144 4>;
-	dma-mask = <0xffffffff>;
 };
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 9577bfd..b8a8284 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -46,8 +46,6 @@ static const struct of_device_id denali_nand_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
 
-static u64 denali_dma_mask;
-
 static int denali_dt_probe(struct platform_device *ofdev)
 {
 	struct resource *denali_reg, *nand_data;
@@ -83,13 +81,6 @@ static int denali_dt_probe(struct platform_device *ofdev)
 	if (IS_ERR(denali->flash_mem))
 		return PTR_ERR(denali->flash_mem);
 
-	if (!of_property_read_u32(ofdev->dev.of_node,
-		"dma-mask", (u32 *)&denali_dma_mask)) {
-		denali->dev->dma_mask = &denali_dma_mask;
-	} else {
-		denali->dev->dma_mask = NULL;
-	}
-
 	dt->clk = devm_clk_get(&ofdev->dev, NULL);
 	if (IS_ERR(dt->clk)) {
 		dev_err(&ofdev->dev, "no clk available\n");
-- 
2.7.4

[RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Masahiro Yamada]

This driver was originally written for the Intel MRST platform with
several platform specific parameters hard-coded.  Another thing we
need to fix is the hard-coded ECC step size.  Currently, it is
defined as follows:

  #define ECC_SECTOR_SIZE 512

(somehow, it is defined in both denali.c and denali.h)

This must be avoided because the Denali IP supports 1024B ECC size
as well.  The Denali User's Guide also says supporting both 512B and
1024B ECC sectors is possible, though it would require instantiation
of two different ECC circuits.  So, possible cases are:

 [1] only 512B ECC size is supported
 [2] only 1024B ECC size is supported
 [3] both 512B and 1024B ECC sizes are supported

For [3], the actually used ECC size is specified by some registers.

Newer versions of this IP have the following registers:
  CFG_DATA_BLOCK_SIZE       (0x6b0)
  CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
  CFG_NUM_DATA_BLOCKS       (0x6d0)

For those versions, the software should set ecc.size and ecc.steps
to these registers.  Old versions do not have such registers, but
they are "reserved", so write accesses are safely ignored.

This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.

The DT property "nand-ecc-step-size" is still optional; a reasonable
default will be chosen for [1] and [2].  For case [3], if exists, it
is recommended to specify the desired ECC size explicitly.

Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---

Changes in v2:
  - Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
  - Make ECC 512 cap and ECC 1024 cap independent
  - Set up three CFG_... registers

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
 drivers/mtd/nand/denali.c                          | 44 +++++++++++++++-------
 drivers/mtd/nand/denali.h                          | 12 +++++-
 drivers/mtd/nand/denali_dt.c                       |  3 +-
 drivers/mtd/nand/denali_pci.c                      |  2 +
 5 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index e593bbe..25313c7 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -7,6 +7,11 @@ Required properties:
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
 
+Optional properties:
+  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to:
+      512   for "altr,socfpga-denali-nand"
+    see nand.txt for details.
+
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
 
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index a190cb2..cf8daba 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -875,8 +875,6 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
 	return max_bitflips;
 }
 
-#define ECC_SECTOR_SIZE 512
-
 #define ECC_SECTOR(x)	(((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
 #define ECC_BYTE(x)	(((x) & ECC_ERROR_ADDRESS__OFFSET))
 #define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
@@ -887,6 +885,7 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
 static int denali_sw_ecc_fixup(struct mtd_info *mtd,
 			       struct denali_nand_info *denali, uint8_t *buf)
 {
+	unsigned int ecc_size = denali->nand.ecc.size;
 	unsigned int bitflips = 0;
 	unsigned int max_bitflips = 0;
 	unsigned int total_bitflips = 0;
@@ -914,9 +913,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
 
 		if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
 			ret = -EBADMSG;
-		} else if (err_byte < ECC_SECTOR_SIZE) {
+		} else if (err_byte < ecc_size) {
 			/*
-			 * If err_byte is larger than ECC_SECTOR_SIZE, means error
+			 * If err_byte is larger than ecc_size, means error
 			 * happened in OOB, so we ignore it. It's no need for
 			 * us to correct it err_device is represented the NAND
 			 * error bits are happened in if there are more than
@@ -925,7 +924,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
 			int offset;
 			unsigned int flips_in_byte;
 
-			offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
+			offset = (err_sector * ecc_size + err_byte) *
 						denali->devnum + err_device;
 
 			/* correct the ECC error */
@@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
 	/* no subpage writes on denali */
 	chip->options |= NAND_NO_SUBPAGE_WRITE;
 
+	if (!chip->ecc.size) {
+		if (denali->caps & DENALI_CAP_ECC_SIZE_512)
+			chip->ecc.size = 512;
+		if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
+			chip->ecc.size = 1024;
+		if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
+			goto failed_req_irq;
+	}
+
+	if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
+	    (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
+	    (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
+		dev_err(denali->dev, "specified ECC size %d in not supported",
+			chip->ecc.size);
+		goto failed_req_irq;
+	}
+
 	/*
 	 * Denali Controller only support 15bit and 8bit ECC in MRST,
 	 * so just let controller do 15bit ECC for MLC and 8bit ECC for
 	 * SLC if possible.
 	 * */
 	if (!nand_is_slc(chip) &&
-			(mtd->oobsize > (denali->bbtskipbytes +
-			ECC_15BITS * (mtd->writesize /
-			ECC_SECTOR_SIZE)))) {
+			mtd->oobsize > denali->bbtskipbytes +
+			ECC_15BITS * (mtd->writesize / chip->ecc.size)) {
 		/* if MLC OOB size is large enough, use 15bit ECC*/
 		chip->ecc.strength = 15;
 		chip->ecc.bytes = ECC_15BITS;
 		iowrite32(15, denali->flash_reg + ECC_CORRECTION);
-	} else if (mtd->oobsize < (denali->bbtskipbytes +
-			ECC_8BITS * (mtd->writesize /
-			ECC_SECTOR_SIZE))) {
+	} else if (mtd->oobsize <
+		   denali->bbtskipbytes + ECC_8BITS * (mtd->writesize / chip->ecc.size)) {
 		pr_err("Your NAND chip OOB is not large enough to contain 8bit ECC correction codes");
 		goto failed_req_irq;
 	} else {
@@ -1603,10 +1617,14 @@ int denali_init(struct denali_nand_info *denali)
 		iowrite32(8, denali->flash_reg + ECC_CORRECTION);
 	}
 
+	iowrite32(chip->ecc.size, denali->flash_reg + CFG_DATA_BLOCK_SIZE);
+	iowrite32(chip->ecc.size, denali->flash_reg + CFG_LAST_DATA_BLOCK_SIZE);
+	/* chip->ecc.steps is set by nand_scan_tail(); not available here */
+	iowrite32(mtd->writesize / chip->ecc.size,
+		  denali->flash_reg + CFG_NUM_DATA_BLOCKS);
+
 	mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
 
-	/* override the default read operations */
-	chip->ecc.size = ECC_SECTOR_SIZE;
 	chip->ecc.read_page = denali_read_page;
 	chip->ecc.read_page_raw = denali_read_page_raw;
 	chip->ecc.write_page = denali_write_page;
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index d9232e4..aa6548a 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -270,6 +270,14 @@
 #define     ECC_COR_INFO__MAX_ERRORS			0x007f
 #define     ECC_COR_INFO__UNCOR_ERR			0x0080
 
+#define CFG_DATA_BLOCK_SIZE			0x6b0
+
+#define CFG_LAST_DATA_BLOCK_SIZE		0x6c0
+
+#define CFG_NUM_DATA_BLOCKS			0x6d0
+
+#define CFG_META_DATA_SIZE			0x6e0
+
 #define DMA_ENABLE				0x700
 #define     DMA_ENABLE__FLAG				0x0001
 
@@ -312,8 +320,6 @@
 #define MODE_10    0x08000000
 #define MODE_11    0x0C000000
 
-#define ECC_SECTOR_SIZE     512
-
 struct nand_buf {
 	int head;
 	int tail;
@@ -350,6 +356,8 @@ struct denali_nand_info {
 	unsigned int caps;
 #define DENALI_CAP_HW_ECC_FIXUP			BIT(0)
 #define DENALI_CAP_DMA_64BIT			BIT(1)
+#define DENALI_CAP_ECC_SIZE_512			BIT(2)
+#define DENALI_CAP_ECC_SIZE_1024		BIT(3)
 };
 
 extern int denali_init(struct denali_nand_info *denali);
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index df9ef36..1681a30 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -35,7 +35,8 @@ struct denali_dt_data {
 };
 
 static const struct denali_dt_data denali_socfpga_data = {
-	.caps = DENALI_CAP_HW_ECC_FIXUP,
+	.caps = DENALI_CAP_HW_ECC_FIXUP |
+		DENALI_CAP_ECC_SIZE_512,
 };
 
 static const struct of_device_id denali_nand_dt_ids[] = {
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index ac84323..5202a11 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -85,6 +85,8 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		goto failed_remap_reg;
 	}
 
+	denali->caps |= DENALI_CAP_ECC_SIZE_512;
+
 	ret = denali_init(denali);
 	if (ret)
 		goto failed_remap_mem;
-- 
2.7.4

--
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: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Boris Brezillon]

On Thu, 23 Mar 2017 05:07:25 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> This driver was originally written for the Intel MRST platform with
> several platform specific parameters hard-coded.  Another thing we
> need to fix is the hard-coded ECC step size.  Currently, it is
> defined as follows:
> 
>   #define ECC_SECTOR_SIZE 512
> 
> (somehow, it is defined in both denali.c and denali.h)
> 
> This must be avoided because the Denali IP supports 1024B ECC size
> as well.  The Denali User's Guide also says supporting both 512B and
> 1024B ECC sectors is possible, though it would require instantiation
> of two different ECC circuits.  So, possible cases are:
> 
>  [1] only 512B ECC size is supported
>  [2] only 1024B ECC size is supported
>  [3] both 512B and 1024B ECC sizes are supported
> 
> For [3], the actually used ECC size is specified by some registers.
> 
> Newer versions of this IP have the following registers:
>   CFG_DATA_BLOCK_SIZE       (0x6b0)
>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
>   CFG_NUM_DATA_BLOCKS       (0x6d0)
> 
> For those versions, the software should set ecc.size and ecc.steps
> to these registers.  Old versions do not have such registers, but
> they are "reserved", so write accesses are safely ignored.
> 
> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
> 
> The DT property "nand-ecc-step-size" is still optional; a reasonable
> default will be chosen for [1] and [2].  For case [3], if exists, it
> is recommended to specify the desired ECC size explicitly.

Actually, the NAND chip gives some hints to help controller drivers
decide which ecc-block-size/strength is appropriate
(chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
nand-ecc-step-size is unneeded (unless you want to force a specific
setting).

> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> Changes in v2:
>   - Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
>   - Make ECC 512 cap and ECC 1024 cap independent
>   - Set up three CFG_... registers
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
>  drivers/mtd/nand/denali.c                          | 44 +++++++++++++++-------
>  drivers/mtd/nand/denali.h                          | 12 +++++-
>  drivers/mtd/nand/denali_dt.c                       |  3 +-
>  drivers/mtd/nand/denali_pci.c                      |  2 +
>  5 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index e593bbe..25313c7 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -7,6 +7,11 @@ Required properties:
>    - reg-names: Should contain the reg names "nand_data" and "denali_reg"
>    - interrupts : The interrupt number.
>  
> +Optional properties:
> +  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to:
> +      512   for "altr,socfpga-denali-nand"
> +    see nand.txt for details.
> +
>  The device tree may optionally contain sub-nodes describing partitions of the
>  address space. See partition.txt for more detail.
>  
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index a190cb2..cf8daba 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -875,8 +875,6 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
>  	return max_bitflips;
>  }
>  
> -#define ECC_SECTOR_SIZE 512
> -
>  #define ECC_SECTOR(x)	(((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
>  #define ECC_BYTE(x)	(((x) & ECC_ERROR_ADDRESS__OFFSET))
>  #define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
> @@ -887,6 +885,7 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
>  static int denali_sw_ecc_fixup(struct mtd_info *mtd,
>  			       struct denali_nand_info *denali, uint8_t *buf)
>  {
> +	unsigned int ecc_size = denali->nand.ecc.size;
>  	unsigned int bitflips = 0;
>  	unsigned int max_bitflips = 0;
>  	unsigned int total_bitflips = 0;
> @@ -914,9 +913,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
>  
>  		if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
>  			ret = -EBADMSG;
> -		} else if (err_byte < ECC_SECTOR_SIZE) {
> +		} else if (err_byte < ecc_size) {
>  			/*
> -			 * If err_byte is larger than ECC_SECTOR_SIZE, means error
> +			 * If err_byte is larger than ecc_size, means error
>  			 * happened in OOB, so we ignore it. It's no need for
>  			 * us to correct it err_device is represented the NAND
>  			 * error bits are happened in if there are more than
> @@ -925,7 +924,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
>  			int offset;
>  			unsigned int flips_in_byte;
>  
> -			offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
> +			offset = (err_sector * ecc_size + err_byte) *
>  						denali->devnum + err_device;
>  
>  			/* correct the ECC error */
> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
>  	/* no subpage writes on denali */
>  	chip->options |= NAND_NO_SUBPAGE_WRITE;
>  
> +	if (!chip->ecc.size) {

You should set it to chip->ecc_step_ds and pick a default value only if
it's still 0 after that. Same goes for ecc.strength.

> +		if (denali->caps & DENALI_CAP_ECC_SIZE_512)
> +			chip->ecc.size = 512;
> +		if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
> +			chip->ecc.size = 1024;
> +		if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
> +			goto failed_req_irq;
> +	}
> +
> +	if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
> +	    (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
> +	    (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
> +		dev_err(denali->dev, "specified ECC size %d in not supported",
> +			chip->ecc.size);
> +		goto failed_req_irq;
> +	}
> +
>  	/*
>  	 * Denali Controller only support 15bit and 8bit ECC in MRST,
>  	 * so just let controller do 15bit ECC for MLC and 8bit ECC for
>  	 * SLC if possible.

Usually the NAND chips expose the ECC requirements, so basing our
decision only on the type of NAND sounds a bit weird.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

Hi Masahiro,

On Thu, 23 Mar 2017 05:06:59 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> It took a couple months to update this series, but finally here is v2.
> (v1: https://lkml.org/lkml/2016/11/26/144 )
> 
> This driver includes many problems.
> 
> One of the biggest one is a bunch of hard-coded parameters.  This IP
> has many parameters that can be customized when a delivery RTL is
> generated.  However, this driver was upstreamed by Intel, with
> Intel parameters hard-coded.  Later, Altera added denali_dt.c to use
> this driver for embedded boards, but they did not fix the code in
> denali.c  So, this driver has never worked.  Even some DT bindings
> actually turned out wrong.
> 
> There are more problems: [1] The driver just retrieves the OOB area as-is
> whereas the controller uses syndrome page layout. [2] Many NAND chip
> specific parameters are hard-coded in the driver. [3] ONFi devices are
> not working  [4] It can not read Bad Block Marker
> 
> This patch series intends to solve those problems.
> 
> Since v1, here are more fixes/improvements:
>  - Fix raw/oob callbacks for syndrome page layout
>  - Implement setup_data_interface() callback
>  - Fix/implement more commands for ONFi devices
>  - Allow to skip the driver internal bounce buffer
>  - Support PIO in case DMA is not supported
>  - Switch from ->cmdfunc over to ->cmd_ctrl

I'll probably pick a few patches for 4.12, so please wait a bit before
sending a v4.

Thanks,

Boris

> 
> 
> Masahiro Yamada (53):
>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
>   mtd: nand: use read_oob() instead of cmdfunc() for bad block check
>   mtd: nand: denali: remove unused CONFIG option and macros
>   mtd: nand: denali: remove redundant define of BANK(x)
>   mtd: nand: denali: remove more unused struct members
>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
>   mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
>   mtd: nand: denali: introduce capability flag
>   mtd: nand: denali: use int where no reason to use fixed width variable
>   mtd: nand: denali: fix erased page checking
>   mtd: nand: denali: fix bitflips calculation in handle_ecc()
>   mtd: nand: denali: support HW_ECC_FIXUP capability
>   mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
>   mtd: nand: denali: support 64bit capable DMA engine
>   mtd: nand: denali_dt: remove dma-mask DT property
>   mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
>   mtd: nand: denali: allow to override revision number
>   mtd: nand: denali: use nand_chip to hold frequently accessed data
>   mtd: nand: denali: call nand_set_flash_node() to set DT node
>   mtd: nand: denali: do not set mtd->name
>   mtd: nand: denali: move multi device fixup code to a helper function
>   mtd: nand: denali: simplify multi device fixup code
>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
>   mtd: nand: denali: remove meaningless writes to read-only registers
>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
>   mtd: nand: denali: support 1024 byte ECC step size
>   mtd: nand: denali: avoid hard-coding ecc.strength and ecc.bytes
>   mtd: nand: denali: support "nand-ecc-strength" DT property
>   mtd: nand: denali: remove Toshiba and Hynix specific fixup code
>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>   mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
>   mtd: nand: denali: use BIT() and GENMASK() for register macros
>   mtd: nand: denali: remove unneeded find_valid_banks()
>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>   mtd: nand: denali: remove meaningless pipeline read-ahead operation
>   mtd: nand: denali: rework interrupt handling
>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>   mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
>   mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)
>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>   mtd: nand: denali: fix bank reset function
>   mtd: nand: denali: use interrupt instead of polling for bank reset
>   mtd: nand: denali: propagate page to helpers via function argument
>   mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
>   mtd: nand: denali: use flag instead of register macro for direction
>   mtd: nand: denali: fix raw and oob accessors for syndrome page layout
>   mtd: nand: denali: support hardware-assisted erased page detection
>   mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
>   mtd: nand: denali: skip driver internal bounce buffer when possible
>   mtd: nand: denali: use non-managed kmalloc() for DMA buffer
>   mtd: nand: denali: enable bad block table scan
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |   24 +-
>  drivers/mtd/nand/Kconfig                           |   11 -
>  drivers/mtd/nand/denali.c                          | 2073 +++++++++-----------
>  drivers/mtd/nand/denali.h                          |  453 ++---
>  drivers/mtd/nand/denali_dt.c                       |  104 +-
>  drivers/mtd/nand/denali_pci.c                      |   10 +-
>  drivers/mtd/nand/nand_base.c                       |   78 +-
>  7 files changed, 1278 insertions(+), 1475 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: [RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-03-23 6:35 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> Hi Masahiro,
>
> On Thu, 23 Mar 2017 05:06:59 +0900
> Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
>> It took a couple months to update this series, but finally here is v2.
>> (v1: https://lkml.org/lkml/2016/11/26/144 )
>>
>> This driver includes many problems.
>>
>> One of the biggest one is a bunch of hard-coded parameters.  This IP
>> has many parameters that can be customized when a delivery RTL is
>> generated.  However, this driver was upstreamed by Intel, with
>> Intel parameters hard-coded.  Later, Altera added denali_dt.c to use
>> this driver for embedded boards, but they did not fix the code in
>> denali.c  So, this driver has never worked.  Even some DT bindings
>> actually turned out wrong.
>>
>> There are more problems: [1] The driver just retrieves the OOB area as-is
>> whereas the controller uses syndrome page layout. [2] Many NAND chip
>> specific parameters are hard-coded in the driver. [3] ONFi devices are
>> not working  [4] It can not read Bad Block Marker
>>
>> This patch series intends to solve those problems.
>>
>> Since v1, here are more fixes/improvements:
>>  - Fix raw/oob callbacks for syndrome page layout
>>  - Implement setup_data_interface() callback
>>  - Fix/implement more commands for ONFi devices
>>  - Allow to skip the driver internal bounce buffer
>>  - Support PIO in case DMA is not supported
>>  - Switch from ->cmdfunc over to ->cmd_ctrl
>
> I'll probably pick a few patches for 4.12, so please wait a bit before
> sending a v4.


This will be helpful.

I was refused by the SMTP server I use
(probably due to too much volume at once).




Please check the following as well.

>>   mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
>>   mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)

I noticed them when I switched from ->cmdfunc to ->cmd_ctrl hook.



-- 
Best Regards
Masahiro Yamada
--
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: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Masahiro Yamada]

Hi Boris,

2017-03-23 6:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 23 Mar 2017 05:07:25 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> This driver was originally written for the Intel MRST platform with
>> several platform specific parameters hard-coded.  Another thing we
>> need to fix is the hard-coded ECC step size.  Currently, it is
>> defined as follows:
>>
>>   #define ECC_SECTOR_SIZE 512
>>
>> (somehow, it is defined in both denali.c and denali.h)
>>
>> This must be avoided because the Denali IP supports 1024B ECC size
>> as well.  The Denali User's Guide also says supporting both 512B and
>> 1024B ECC sectors is possible, though it would require instantiation
>> of two different ECC circuits.  So, possible cases are:
>>
>>  [1] only 512B ECC size is supported
>>  [2] only 1024B ECC size is supported
>>  [3] both 512B and 1024B ECC sizes are supported
>>
>> For [3], the actually used ECC size is specified by some registers.
>>
>> Newer versions of this IP have the following registers:
>>   CFG_DATA_BLOCK_SIZE       (0x6b0)
>>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
>>   CFG_NUM_DATA_BLOCKS       (0x6d0)
>>
>> For those versions, the software should set ecc.size and ecc.steps
>> to these registers.  Old versions do not have such registers, but
>> they are "reserved", so write accesses are safely ignored.
>>
>> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
>>
>> The DT property "nand-ecc-step-size" is still optional; a reasonable
>> default will be chosen for [1] and [2].  For case [3], if exists, it
>> is recommended to specify the desired ECC size explicitly.
>
> Actually, the NAND chip gives some hints to help controller drivers
> decide which ecc-block-size/strength is appropriate
> (chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
> nand-ecc-step-size is unneeded (unless you want to force a specific
> setting).


But, if we look at nand_flash_detect_onfi(),
->ecc_step_ds is almost a fixed value, 512, right?

if (p->ecc_bits != 0xff) {
        chip->ecc_strength_ds = p->ecc_bits;
        chip->ecc_step_ds = 512;


As far as I understood,
->ecc_step_ds is not a hint for drivers to decide ->ecc.size.

Rather, ->ecc_step_ds just specifies the unit of ->ecc_strength_ds.



>>                       int offset;
>>                       unsigned int flips_in_byte;
>>
>> -                     offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
>> +                     offset = (err_sector * ecc_size + err_byte) *
>>                                               denali->devnum + err_device;
>>
>>                       /* correct the ECC error */
>> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
>>       /* no subpage writes on denali */
>>       chip->options |= NAND_NO_SUBPAGE_WRITE;
>>
>> +     if (!chip->ecc.size) {
>
> You should set it to chip->ecc_step_ds and pick a default value only if
> it's still 0 after that. Same goes for ecc.strength.

Sorry, I still do not understand this.

->ecc_strength_ds and ->ecc_step_ds
shows how often bit-flip occurs in this device.

So, nand_ecc_strength_good() is a typical usage of these.

How many sectors the driver actually splits the device into
is a different issue, I think.



>> +             if (denali->caps & DENALI_CAP_ECC_SIZE_512)
>> +                     chip->ecc.size = 512;
>> +             if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
>> +                     chip->ecc.size = 1024;
>> +             if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
>> +                     goto failed_req_irq;
>> +     }
>> +
>> +     if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
>> +         (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
>> +         (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
>> +             dev_err(denali->dev, "specified ECC size %d in not supported",
>> +                     chip->ecc.size);
>> +             goto failed_req_irq;
>> +     }
>> +
>>       /*
>>        * Denali Controller only support 15bit and 8bit ECC in MRST,
>>        * so just let controller do 15bit ECC for MLC and 8bit ECC for
>>        * SLC if possible.
>
> Usually the NAND chips expose the ECC requirements, so basing our
> decision only on the type of NAND sounds a bit weird.


chip->ecc.size is one of the configuration of this controller IP.

SoC vendors choose 512, 1024, or both of them
when they buy this IP.

If both 512 and 1024 are supported, 1024 is usually a better choice
because bigger ecc.size uses ECC more efficiently.


It is unrelated to the chips' requirements.



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Boris Brezillon]

On Thu, 23 Mar 2017 15:53:14 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2017-03-23 6:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 23 Mar 2017 05:07:25 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> This driver was originally written for the Intel MRST platform with
> >> several platform specific parameters hard-coded.  Another thing we
> >> need to fix is the hard-coded ECC step size.  Currently, it is
> >> defined as follows:
> >>
> >>   #define ECC_SECTOR_SIZE 512
> >>
> >> (somehow, it is defined in both denali.c and denali.h)
> >>
> >> This must be avoided because the Denali IP supports 1024B ECC size
> >> as well.  The Denali User's Guide also says supporting both 512B and
> >> 1024B ECC sectors is possible, though it would require instantiation
> >> of two different ECC circuits.  So, possible cases are:
> >>
> >>  [1] only 512B ECC size is supported
> >>  [2] only 1024B ECC size is supported
> >>  [3] both 512B and 1024B ECC sizes are supported
> >>
> >> For [3], the actually used ECC size is specified by some registers.
> >>
> >> Newer versions of this IP have the following registers:
> >>   CFG_DATA_BLOCK_SIZE       (0x6b0)
> >>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
> >>   CFG_NUM_DATA_BLOCKS       (0x6d0)
> >>
> >> For those versions, the software should set ecc.size and ecc.steps
> >> to these registers.  Old versions do not have such registers, but
> >> they are "reserved", so write accesses are safely ignored.
> >>
> >> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
> >>
> >> The DT property "nand-ecc-step-size" is still optional; a reasonable
> >> default will be chosen for [1] and [2].  For case [3], if exists, it
> >> is recommended to specify the desired ECC size explicitly.  
> >
> > Actually, the NAND chip gives some hints to help controller drivers
> > decide which ecc-block-size/strength is appropriate
> > (chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
> > nand-ecc-step-size is unneeded (unless you want to force a specific
> > setting).  
> 
> 
> But, if we look at nand_flash_detect_onfi(),
> ->ecc_step_ds is almost a fixed value, 512, right?  
> 
> if (p->ecc_bits != 0xff) {
>         chip->ecc_strength_ds = p->ecc_bits;
>         chip->ecc_step_ds = 512;
> 

Nope, if the NAND requires a different ECC block size, you will have
0xff in this field and the real ECC requirements will be exposed in the
extended parameter page [1].

> 
> As far as I understood,
> ->ecc_step_ds is not a hint for drivers to decide ->ecc.size.  
> 
> Rather, ->ecc_step_ds just specifies the unit of ->ecc_strength_ds.

->ecc_xxx_ds are encoding the minimum ECC requirements as expressed by
the NAND chip, so yes, it is an hint for the ECC engine configuration.

It does not necessarily means it has to be exactly the same, but it
should be used to determine which setting is appropriate. For example,
if the NAND says it requires a minimum of 4bits/512bytes but your
controller only supports 16bits/1024bytes, then it's fine.



> 
> 
> 
> >>                       int offset;
> >>                       unsigned int flips_in_byte;
> >>
> >> -                     offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
> >> +                     offset = (err_sector * ecc_size + err_byte) *
> >>                                               denali->devnum + err_device;
> >>
> >>                       /* correct the ECC error */
> >> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
> >>       /* no subpage writes on denali */
> >>       chip->options |= NAND_NO_SUBPAGE_WRITE;
> >>
> >> +     if (!chip->ecc.size) {  
> >
> > You should set it to chip->ecc_step_ds and pick a default value only if
> > it's still 0 after that. Same goes for ecc.strength.  
> 
> Sorry, I still do not understand this.
> 
> ->ecc_strength_ds and ->ecc_step_ds  
> shows how often bit-flip occurs in this device.

It represents the minimum ECC requirements to ensure a 'reliable' setup.

> 
> So, nand_ecc_strength_good() is a typical usage of these.

nand_ecc_strength_good() is complaining if you choose an ECC setting
that is below the minimum requirements.

> 
> How many sectors the driver actually splits the device into
> is a different issue, I think.

The choice is left to the ECC controller, but it should take the
->ecc_strength_ds and ->ecc_step_ds information into account when
choosing the ECC settings.

> 
> 
> 
> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_512)
> >> +                     chip->ecc.size = 512;
> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
> >> +                     chip->ecc.size = 1024;
> >> +             if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
> >> +                     goto failed_req_irq;
> >> +     }
> >> +
> >> +     if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
> >> +         (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
> >> +         (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
> >> +             dev_err(denali->dev, "specified ECC size %d in not supported",
> >> +                     chip->ecc.size);
> >> +             goto failed_req_irq;
> >> +     }
> >> +
> >>       /*
> >>        * Denali Controller only support 15bit and 8bit ECC in MRST,
> >>        * so just let controller do 15bit ECC for MLC and 8bit ECC for
> >>        * SLC if possible.  
> >
> > Usually the NAND chips expose the ECC requirements, so basing our
> > decision only on the type of NAND sounds a bit weird.  
> 
> 
> chip->ecc.size is one of the configuration of this controller IP.
> 
> SoC vendors choose 512, 1024, or both of them
> when they buy this IP.

Yes, and that's not a problem.

> 
> If both 512 and 1024 are supported, 1024 is usually a better choice
> because bigger ecc.size uses ECC more efficiently.

We agree.

> 
> 
> It is unrelated to the chips' requirements.

It is related to the chip requirements.
Say you have a chip that requires a minimum of 4bits/512bytes. If you
want to convert that to a 1024byte block setting it's perfectly fine,
but then you'll have to meet (2 * ->ecc_strength_ds) for the
ecc.strength parameter.

The nand-ecc-strength and nand-ecc-step DT properties are here to
override the chip requirements and force a specific setting. This is
for example needed when the bootloader hardcodes an ECC setting without
taking the NAND chip requirements into account, and since you want to
read/write from both the bootloader and linux, you'll have to force this
specific ECC setting, but this case should be the exception, not the
default behavior.

If you want an example on how to extrapolate ECC engine settings from
->ecc_xxx_ds info, you can look at the sunxi_nand driver [2].

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L3491
[2]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1946

Re: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Masahiro Yamada]

Hi Boris,


2017-03-23 17:39 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 23 Mar 2017 15:53:14 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-03-23 6:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Thu, 23 Mar 2017 05:07:25 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> This driver was originally written for the Intel MRST platform with
>> >> several platform specific parameters hard-coded.  Another thing we
>> >> need to fix is the hard-coded ECC step size.  Currently, it is
>> >> defined as follows:
>> >>
>> >>   #define ECC_SECTOR_SIZE 512
>> >>
>> >> (somehow, it is defined in both denali.c and denali.h)
>> >>
>> >> This must be avoided because the Denali IP supports 1024B ECC size
>> >> as well.  The Denali User's Guide also says supporting both 512B and
>> >> 1024B ECC sectors is possible, though it would require instantiation
>> >> of two different ECC circuits.  So, possible cases are:
>> >>
>> >>  [1] only 512B ECC size is supported
>> >>  [2] only 1024B ECC size is supported
>> >>  [3] both 512B and 1024B ECC sizes are supported
>> >>
>> >> For [3], the actually used ECC size is specified by some registers.
>> >>
>> >> Newer versions of this IP have the following registers:
>> >>   CFG_DATA_BLOCK_SIZE       (0x6b0)
>> >>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
>> >>   CFG_NUM_DATA_BLOCKS       (0x6d0)
>> >>
>> >> For those versions, the software should set ecc.size and ecc.steps
>> >> to these registers.  Old versions do not have such registers, but
>> >> they are "reserved", so write accesses are safely ignored.
>> >>
>> >> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
>> >>
>> >> The DT property "nand-ecc-step-size" is still optional; a reasonable
>> >> default will be chosen for [1] and [2].  For case [3], if exists, it
>> >> is recommended to specify the desired ECC size explicitly.
>> >
>> > Actually, the NAND chip gives some hints to help controller drivers
>> > decide which ecc-block-size/strength is appropriate
>> > (chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
>> > nand-ecc-step-size is unneeded (unless you want to force a specific
>> > setting).
>>
>>
>> But, if we look at nand_flash_detect_onfi(),
>> ->ecc_step_ds is almost a fixed value, 512, right?
>>
>> if (p->ecc_bits != 0xff) {
>>         chip->ecc_strength_ds = p->ecc_bits;
>>         chip->ecc_step_ds = 512;
>>
>
> Nope, if the NAND requires a different ECC block size, you will have
> 0xff in this field and the real ECC requirements will be exposed in the
> extended parameter page [1].
>
>>
>> As far as I understood,
>> ->ecc_step_ds is not a hint for drivers to decide ->ecc.size.
>>
>> Rather, ->ecc_step_ds just specifies the unit of ->ecc_strength_ds.
>
> ->ecc_xxx_ds are encoding the minimum ECC requirements as expressed by
> the NAND chip, so yes, it is an hint for the ECC engine configuration.
>
> It does not necessarily means it has to be exactly the same, but it
> should be used to determine which setting is appropriate. For example,
> if the NAND says it requires a minimum of 4bits/512bytes but your
> controller only supports 16bits/1024bytes, then it's fine.
>
>
>
>>
>>
>>
>> >>                       int offset;
>> >>                       unsigned int flips_in_byte;
>> >>
>> >> -                     offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
>> >> +                     offset = (err_sector * ecc_size + err_byte) *
>> >>                                               denali->devnum + err_device;
>> >>
>> >>                       /* correct the ECC error */
>> >> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
>> >>       /* no subpage writes on denali */
>> >>       chip->options |= NAND_NO_SUBPAGE_WRITE;
>> >>
>> >> +     if (!chip->ecc.size) {
>> >
>> > You should set it to chip->ecc_step_ds and pick a default value only if
>> > it's still 0 after that. Same goes for ecc.strength.
>>
>> Sorry, I still do not understand this.
>>
>> ->ecc_strength_ds and ->ecc_step_ds
>> shows how often bit-flip occurs in this device.
>
> It represents the minimum ECC requirements to ensure a 'reliable' setup.
>
>>
>> So, nand_ecc_strength_good() is a typical usage of these.
>
> nand_ecc_strength_good() is complaining if you choose an ECC setting
> that is below the minimum requirements.
>
>>
>> How many sectors the driver actually splits the device into
>> is a different issue, I think.
>
> The choice is left to the ECC controller, but it should take the
> ->ecc_strength_ds and ->ecc_step_ds information into account when
> choosing the ECC settings.
>
>>
>>
>>
>> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_512)
>> >> +                     chip->ecc.size = 512;
>> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
>> >> +                     chip->ecc.size = 1024;
>> >> +             if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
>> >> +                     goto failed_req_irq;
>> >> +     }
>> >> +
>> >> +     if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
>> >> +         (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
>> >> +         (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
>> >> +             dev_err(denali->dev, "specified ECC size %d in not supported",
>> >> +                     chip->ecc.size);
>> >> +             goto failed_req_irq;
>> >> +     }
>> >> +
>> >>       /*
>> >>        * Denali Controller only support 15bit and 8bit ECC in MRST,
>> >>        * so just let controller do 15bit ECC for MLC and 8bit ECC for
>> >>        * SLC if possible.
>> >
>> > Usually the NAND chips expose the ECC requirements, so basing our
>> > decision only on the type of NAND sounds a bit weird.
>>
>>
>> chip->ecc.size is one of the configuration of this controller IP.
>>
>> SoC vendors choose 512, 1024, or both of them
>> when they buy this IP.
>
> Yes, and that's not a problem.
>
>>
>> If both 512 and 1024 are supported, 1024 is usually a better choice
>> because bigger ecc.size uses ECC more efficiently.
>
> We agree.
>
>>
>>
>> It is unrelated to the chips' requirements.
>
> It is related to the chip requirements.
> Say you have a chip that requires a minimum of 4bits/512bytes. If you
> want to convert that to a 1024byte block setting it's perfectly fine,
> but then you'll have to meet (2 * ->ecc_strength_ds) for the
> ecc.strength parameter.

I think this example case is always fine from the
"bigger ecc.size uses ECC more efficiently" we agreed.
If 4bits/512bytes is achievable, 8bits/1024bytes is always met.


The reverse is not always true.
If the chip requires 8bits/1024bytes then controller uses 4bit/512bytes,
this could be a reliability problem.



> The nand-ecc-strength and nand-ecc-step DT properties are here to
> override the chip requirements and force a specific setting. This is
> for example needed when the bootloader hardcodes an ECC setting without
> taking the NAND chip requirements into account, and since you want to
> read/write from both the bootloader and linux, you'll have to force this
> specific ECC setting, but this case should be the exception, not the
> default behavior.

Yes, I also thought the case where the boot-loader hardcodes an ECC setting.

Moreover, the Boot ROM really hard-codes (hard-wires)
the ECC setting in some cases.   On some Socionext UniPhier boards,
users have no freedom to change the ECC settings.

So, DT property need to be supported somehow.


>
> If you want an example on how to extrapolate ECC engine settings from
> ->ecc_xxx_ds info, you can look at the sunxi_nand driver [2].
>
> [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L3491
> [2]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1946
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Boris Brezillon]

On Fri, 24 Mar 2017 12:23:01 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> >  
> >>
> >>
> >> It is unrelated to the chips' requirements.  
> >
> > It is related to the chip requirements.
> > Say you have a chip that requires a minimum of 4bits/512bytes. If you
> > want to convert that to a 1024byte block setting it's perfectly fine,
> > but then you'll have to meet (2 * ->ecc_strength_ds) for the
> > ecc.strength parameter.  
> 
> I think this example case is always fine from the
> "bigger ecc.size uses ECC more efficiently" we agreed.
> If 4bits/512bytes is achievable, 8bits/1024bytes is always met.
> 
> 
> The reverse is not always true.
> If the chip requires 8bits/1024bytes then controller uses 4bit/512bytes,
> this could be a reliability problem.

Yes. In this case, you can choose 8bits/512byte if your controller
supports it and the NAND has enough OOB bytes to store the ECC.
Otherwise, you try to do you best and pick 4bits/512bytes even if it's
a bit less reliable than 8bits/1024bytes.

> 
> 
> 
> > The nand-ecc-strength and nand-ecc-step DT properties are here to
> > override the chip requirements and force a specific setting. This is
> > for example needed when the bootloader hardcodes an ECC setting without
> > taking the NAND chip requirements into account, and since you want to
> > read/write from both the bootloader and linux, you'll have to force this
> > specific ECC setting, but this case should be the exception, not the
> > default behavior.  
> 
> Yes, I also thought the case where the boot-loader hardcodes an ECC setting.
> 
> Moreover, the Boot ROM really hard-codes (hard-wires)
> the ECC setting in some cases.   On some Socionext UniPhier boards,
> users have no freedom to change the ECC settings.

Okay, in this case there's nothing you can choose indeed.

> 
> So, DT property need to be supported somehow.

There's a difference between supporting the DT props and making them
mandatory. I never suggested to get rid of DT settings, just to use
NAND requirements when nothing is specified in the DT.

Anyway, if you want to keep this behavior, you should state in the
bindings doc that nand-ecc-step-size is mandatory for controllers
where the ECC block size is configurable (unless you don't support
such controllers yet).

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

Hi Masahiro,

On Thu, 23 Mar 2017 05:06:59 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> It took a couple months to update this series, but finally here is v2.
> (v1: https://lkml.org/lkml/2016/11/26/144 )
> 
> This driver includes many problems.
> 
> One of the biggest one is a bunch of hard-coded parameters.  This IP
> has many parameters that can be customized when a delivery RTL is
> generated.  However, this driver was upstreamed by Intel, with
> Intel parameters hard-coded.  Later, Altera added denali_dt.c to use
> this driver for embedded boards, but they did not fix the code in
> denali.c  So, this driver has never worked.  Even some DT bindings
> actually turned out wrong.
> 
> There are more problems: [1] The driver just retrieves the OOB area as-is
> whereas the controller uses syndrome page layout. [2] Many NAND chip
> specific parameters are hard-coded in the driver. [3] ONFi devices are
> not working  [4] It can not read Bad Block Marker
> 
> This patch series intends to solve those problems.
> 
> Since v1, here are more fixes/improvements:
>  - Fix raw/oob callbacks for syndrome page layout
>  - Implement setup_data_interface() callback
>  - Fix/implement more commands for ONFi devices
>  - Allow to skip the driver internal bounce buffer
>  - Support PIO in case DMA is not supported
>  - Switch from ->cmdfunc over to ->cmd_ctrl
> 
> 
> Masahiro Yamada (53):
>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
>   mtd: nand: use read_oob() instead of cmdfunc() for bad block check
>   mtd: nand: denali: remove unused CONFIG option and macros
>   mtd: nand: denali: remove redundant define of BANK(x)
>   mtd: nand: denali: remove more unused struct members
>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
>   mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
>   mtd: nand: denali: introduce capability flag
>   mtd: nand: denali: use int where no reason to use fixed width variable
>   mtd: nand: denali: fix erased page checking
>   mtd: nand: denali: fix bitflips calculation in handle_ecc()
>   mtd: nand: denali: support HW_ECC_FIXUP capability
>   mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
>   mtd: nand: denali: support 64bit capable DMA engine
>   mtd: nand: denali_dt: remove dma-mask DT property
>   mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
>   mtd: nand: denali: allow to override revision number
>   mtd: nand: denali: use nand_chip to hold frequently accessed data
>   mtd: nand: denali: call nand_set_flash_node() to set DT node
>   mtd: nand: denali: do not set mtd->name
>   mtd: nand: denali: move multi device fixup code to a helper function
>   mtd: nand: denali: simplify multi device fixup code
>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
>   mtd: nand: denali: remove meaningless writes to read-only registers
>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
>   mtd: nand: denali: support 1024 byte ECC step size
>   mtd: nand: denali: avoid hard-coding ecc.strength and ecc.bytes
>   mtd: nand: denali: support "nand-ecc-strength" DT property
>   mtd: nand: denali: remove Toshiba and Hynix specific fixup code
>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>   mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
>   mtd: nand: denali: use BIT() and GENMASK() for register macros
>   mtd: nand: denali: remove unneeded find_valid_banks()
>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>   mtd: nand: denali: remove meaningless pipeline read-ahead operation
>   mtd: nand: denali: rework interrupt handling
>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>   mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
>   mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)
>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>   mtd: nand: denali: fix bank reset function
>   mtd: nand: denali: use interrupt instead of polling for bank reset
>   mtd: nand: denali: propagate page to helpers via function argument
>   mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
>   mtd: nand: denali: use flag instead of register macro for direction
>   mtd: nand: denali: fix raw and oob accessors for syndrome page layout
>   mtd: nand: denali: support hardware-assisted erased page detection
>   mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
>   mtd: nand: denali: skip driver internal bounce buffer when possible
>   mtd: nand: denali: use non-managed kmalloc() for DMA buffer
>   mtd: nand: denali: enable bad block table scan

Applied patch 1 to 9, 40 and 41. I'll see what else I can apply so that
you don't have to re-post the remaining 42 patches, but I fear some of
them won't apply correctly without patch 10 on which I had comments.

Thanks,

Boris
--
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: [RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,



2017-03-25 5:13 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:

>>
>> Masahiro Yamada (53):
>>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
>>   mtd: nand: use read_oob() instead of cmdfunc() for bad block check
>>   mtd: nand: denali: remove unused CONFIG option and macros
>>   mtd: nand: denali: remove redundant define of BANK(x)
>>   mtd: nand: denali: remove more unused struct members
>>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
>>   mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
>>   mtd: nand: denali: introduce capability flag
>>   mtd: nand: denali: use int where no reason to use fixed width variable
>>   mtd: nand: denali: fix erased page checking
>>   mtd: nand: denali: fix bitflips calculation in handle_ecc()
>>   mtd: nand: denali: support HW_ECC_FIXUP capability
>>   mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
>>   mtd: nand: denali: support 64bit capable DMA engine
>>   mtd: nand: denali_dt: remove dma-mask DT property
>>   mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
>>   mtd: nand: denali: allow to override revision number
>>   mtd: nand: denali: use nand_chip to hold frequently accessed data
>>   mtd: nand: denali: call nand_set_flash_node() to set DT node
>>   mtd: nand: denali: do not set mtd->name
>>   mtd: nand: denali: move multi device fixup code to a helper function
>>   mtd: nand: denali: simplify multi device fixup code
>>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
>>   mtd: nand: denali: remove meaningless writes to read-only registers
>>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
>>   mtd: nand: denali: support 1024 byte ECC step size
>>   mtd: nand: denali: avoid hard-coding ecc.strength and ecc.bytes
>>   mtd: nand: denali: support "nand-ecc-strength" DT property
>>   mtd: nand: denali: remove Toshiba and Hynix specific fixup code
>>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>>   mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
>>   mtd: nand: denali: use BIT() and GENMASK() for register macros
>>   mtd: nand: denali: remove unneeded find_valid_banks()
>>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>>   mtd: nand: denali: remove meaningless pipeline read-ahead operation
>>   mtd: nand: denali: rework interrupt handling
>>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>>   mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
>>   mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)
>>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>>   mtd: nand: denali: fix bank reset function
>>   mtd: nand: denali: use interrupt instead of polling for bank reset
>>   mtd: nand: denali: propagate page to helpers via function argument
>>   mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
>>   mtd: nand: denali: use flag instead of register macro for direction
>>   mtd: nand: denali: fix raw and oob accessors for syndrome page layout
>>   mtd: nand: denali: support hardware-assisted erased page detection
>>   mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
>>   mtd: nand: denali: skip driver internal bounce buffer when possible
>>   mtd: nand: denali: use non-managed kmalloc() for DMA buffer
>>   mtd: nand: denali: enable bad block table scan
>
> Applied patch 1 to 9, 40 and 41. I'll see what else I can apply so that
> you don't have to re-post the remaining 42 patches, but I fear some of
> them won't apply correctly without patch 10 on which I had comments.

Thanks!

This will be very helpful when sending the next version.

If you like, you can also apply the following 9 patches cleanly in this order.

18/53 mtd: nand: denali: use nand_chip to hold frequently accessed data
19/53 mtd: nand: denali: call nand_set_flash_node() to set DT node
20/53 mtd: nand: denali: do not set mtd->name
21/53 mtd: nand: denali: move multi device fixup code to a helper function
22/53 mtd: nand: denali: simplify multi device fixup code
23/53 mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
24/53 mtd: nand: denali: remove meaningless writes to read-only registers
25/53 mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
36/53 mtd: nand: denali: remove meaningless pipeline read-ahead operation

These are less controversial, and have no dependency on 10/53-17/53.




50/53 "mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset"
is a change to the NAND framework.  (used as a pre-requisite for 51/53)

Surely no dependency on the others, but If you wait more for comments
from other developers, that's no problem.



I will update the commented parts,
and hopefully send v3 next week.

Thanks!


-- 
Best Regards
Masahiro Yamada

Re: [RESEND PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

On Sat, 25 Mar 2017 23:40:38 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 
> 2017-03-25 5:13 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> 
> >>
> >> Masahiro Yamada (53):
> >>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
> >>   mtd: nand: use read_oob() instead of cmdfunc() for bad block check
> >>   mtd: nand: denali: remove unused CONFIG option and macros
> >>   mtd: nand: denali: remove redundant define of BANK(x)
> >>   mtd: nand: denali: remove more unused struct members
> >>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
> >>   mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
> >>   mtd: nand: denali: introduce capability flag
> >>   mtd: nand: denali: use int where no reason to use fixed width variable
> >>   mtd: nand: denali: fix erased page checking
> >>   mtd: nand: denali: fix bitflips calculation in handle_ecc()
> >>   mtd: nand: denali: support HW_ECC_FIXUP capability
> >>   mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
> >>   mtd: nand: denali: support 64bit capable DMA engine
> >>   mtd: nand: denali_dt: remove dma-mask DT property
> >>   mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
> >>   mtd: nand: denali: allow to override revision number
> >>   mtd: nand: denali: use nand_chip to hold frequently accessed data
> >>   mtd: nand: denali: call nand_set_flash_node() to set DT node
> >>   mtd: nand: denali: do not set mtd->name
> >>   mtd: nand: denali: move multi device fixup code to a helper function
> >>   mtd: nand: denali: simplify multi device fixup code
> >>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
> >>   mtd: nand: denali: remove meaningless writes to read-only registers
> >>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
> >>   mtd: nand: denali: support 1024 byte ECC step size
> >>   mtd: nand: denali: avoid hard-coding ecc.strength and ecc.bytes
> >>   mtd: nand: denali: support "nand-ecc-strength" DT property
> >>   mtd: nand: denali: remove Toshiba and Hynix specific fixup code
> >>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
> >>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
> >>   mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
> >>   mtd: nand: denali: use BIT() and GENMASK() for register macros
> >>   mtd: nand: denali: remove unneeded find_valid_banks()
> >>   mtd: nand: denali: handle timing parameters by setup_data_interface()
> >>   mtd: nand: denali: remove meaningless pipeline read-ahead operation
> >>   mtd: nand: denali: rework interrupt handling
> >>   mtd: nand: denali: fix NAND_CMD_STATUS handling
> >>   mtd: nand: denali: fix NAND_CMD_PARAM handling
> >>   mtd: nand: do not check R/B# for CMD_READID in nand_command(_lp)
> >>   mtd: nand: do not check R/B# for CMD_SET_FEATURES in nand_command(_lp)
> >>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
> >>   mtd: nand: denali: fix bank reset function
> >>   mtd: nand: denali: use interrupt instead of polling for bank reset
> >>   mtd: nand: denali: propagate page to helpers via function argument
> >>   mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
> >>   mtd: nand: denali: use flag instead of register macro for direction
> >>   mtd: nand: denali: fix raw and oob accessors for syndrome page layout
> >>   mtd: nand: denali: support hardware-assisted erased page detection
> >>   mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
> >>   mtd: nand: denali: skip driver internal bounce buffer when possible
> >>   mtd: nand: denali: use non-managed kmalloc() for DMA buffer
> >>   mtd: nand: denali: enable bad block table scan  
> >
> > Applied patch 1 to 9, 40 and 41. I'll see what else I can apply so that
> > you don't have to re-post the remaining 42 patches, but I fear some of
> > them won't apply correctly without patch 10 on which I had comments.  
> 
> Thanks!
> 
> This will be very helpful when sending the next version.
> 
> If you like, you can also apply the following 9 patches cleanly in this order.
> 
> 18/53 mtd: nand: denali: use nand_chip to hold frequently accessed data
> 19/53 mtd: nand: denali: call nand_set_flash_node() to set DT node
> 20/53 mtd: nand: denali: do not set mtd->name
> 21/53 mtd: nand: denali: move multi device fixup code to a helper function
> 22/53 mtd: nand: denali: simplify multi device fixup code
> 23/53 mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
> 24/53 mtd: nand: denali: remove meaningless writes to read-only registers
> 25/53 mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION

Applied all the above patches.

> 36/53 mtd: nand: denali: remove meaningless pipeline read-ahead operation

Still need to review this one.

> 
> These are less controversial, and have no dependency on 10/53-17/53.
> 
> 
> 
> 
> 50/53 "mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset"
> is a change to the NAND framework.  (used as a pre-requisite for 51/53)

Had a comment on the commit message (not the patch content). I find it
too alarmist: you seem to imply that all controller drivers doing DMA
on nand buffers allocated by the core are broken, which I think is not
the case.
I don't deny the theoretical risk of using non-cache aligned buffers,
but in practice, given the workflow we have in the NAND framework, I
don't think cache management operations can trigger data corruptions (I
might be wrong, though).

Re: [RESEND PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant

[Rob Herring]

On Thu, Mar 23, 2017 at 05:07:12AM +0900, Masahiro Yamada wrote:
> There are various customizable parameters, so several variants for
> this IP.  A generic compatible like "denali,denali-nand-dt" is
> useless.  Moreover, there are multiple things wrong with this string.
> (Refer to Rob's comment [1])
> 
> The denali_dt.c was split out and this compatible was added by Altera
> for the SOCFPGA port.  So, replace it with a more specific string.
> 
> The Denali IP on SOCFPGA incorporates the hardware ECC fixup feature.
> So, this capability should be associated with the compatible string.
> 
> [1] https://lkml.org/lkml/2016/12/1/450
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2:
>   - Add caps flag to of_device_id data, not hard-code in driver code
>   - Add Altera-specific compatible.
> 
>  Documentation/devicetree/bindings/mtd/denali-nand.txt |  5 +++--
>  drivers/mtd/nand/denali_dt.c                          | 14 ++++++++++----
>  2 files changed, 13 insertions(+), 6 deletions(-)

You should also mention there are no users (in-tree) of the old string.

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/