[PATCH v3 00/37] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[PATCH v3 00/37] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

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.

Outstanding changes are:
- 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

18 patches were merged at v2.
Here is the rest of the series.

v1: https://lkml.org/lkml/2016/11/26/144
v2: https://lkml.org/lkml/2017/3/22/804


Masahiro Yamada (37):
  mtd: nand: relax ecc.read_page() return value for uncorrectable ECC
  mtd: nand: denali: allow to override mtd->name from label DT property
  mtd: nand: denali: remove meaningless pipeline read-ahead operation
  mtd: nand: denali: fix bitflips calculation in handle_ecc()
  mtd: nand: denali: fix erased page checking
  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: 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: rework interrupt handling
  mtd: nand: denali: fix NAND_CMD_STATUS handling
  mtd: nand: denali: fix NAND_CMD_PARAM handling
  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: allow drivers to request minimum alignment for passed
    buffer
  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/denali.c                          | 1971 ++++++++++----------
 drivers/mtd/nand/denali.h                          |  308 +--
 drivers/mtd/nand/denali_dt.c                       |   90 +-
 drivers/mtd/nand/denali_pci.c                      |   10 +-
 drivers/mtd/nand/nand_base.c                       |   49 +-
 include/linux/mtd/nand.h                           |    4 +-
 7 files changed, 1234 insertions(+), 1222 deletions(-)

-- 
2.7.4


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

[PATCH v3 07/37] 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,denali-nand-dt" was added by Altera for the SOCFPGA port.
Replace it with a more specific string "altr,socfpga-denali-nand".
There are no users (in upstream) of the old compatible string.

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

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

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

Changes in v3:
  - Update git-log to mention there are no users of the old binding.

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

[PATCH v3 09/37] 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 v3: None
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

[PATCH v3 12/37] 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

Newer versions of this IP need ecc.size and ecc.steps explicitly
set up via the following registers:
  CFG_DATA_BLOCK_SIZE       (0x6b0)
  CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
  CFG_NUM_DATA_BLOCKS       (0x6d0)

Older versions do not have such registers (they were 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], users can
force ECC size via DT in case firmware hard-codes ECC settings.
If not specified, the driver will use chip's ECC requirement as a
hint to decide the ECC size.

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

Changes in v3:
  - Move DENALI_CAP_ define out of struct denali_nand_info
  - Use chip->ecc_step_ds as a hint to choose chip->ecc.size
    where possible

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                          | 51 ++++++++++++++++------
 drivers/mtd/nand/denali.h                          | 12 ++++-
 drivers/mtd/nand/denali_dt.c                       |  3 +-
 drivers/mtd/nand/denali_pci.c                      |  2 +
 5 files changed, 57 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 16634df..78d3b18 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -886,8 +886,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)
@@ -899,6 +897,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
 			       struct denali_nand_info *denali,
 			       unsigned long *uncor_ecc_flags, uint8_t *buf)
 {
+	unsigned int ecc_size = denali->nand.ecc.size;
 	unsigned int bitflips = 0;
 	unsigned int max_bitflips = 0;
 	uint32_t err_addr, err_cor_info;
@@ -928,9 +927,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
 			 * an erased sector.
 			 */
 			*uncor_ecc_flags |= BIT(err_sector);
-		} 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
@@ -939,7 +938,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 */
@@ -1587,21 +1586,43 @@ int denali_init(struct denali_nand_info *denali)
 	chip->options |= NAND_NO_SUBPAGE_WRITE;
 
 	/*
+	 * If the controller supports both 512B and 1024B ECC, and DT does not
+	 * specify "nand-ecc-step-size", use the chip's requirement for a hint
+	 * to choose ECC size.
+	 */
+	if (!chip->ecc.size &&
+	    denali->caps & DENALI_CAP_ECC_SIZE_512 &&
+	    denali->caps & DENALI_CAP_ECC_SIZE_1024)
+		chip->ecc.size = chip->ecc_step_ds;
+
+	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 (!(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, "ECC size %d is not supported on this controller",
+			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 {
@@ -1610,10 +1631,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 ec00485..7c24d82 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -265,6 +265,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
 
@@ -307,8 +315,6 @@
 #define MODE_10    0x08000000
 #define MODE_11    0x0C000000
 
-#define ECC_SECTOR_SIZE     512
-
 struct nand_buf {
 	int head;
 	int tail;
@@ -347,6 +353,8 @@ struct denali_nand_info {
 
 #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);
 extern void denali_remove(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

[PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Historically, this driver tried to choose as big ECC strength as
possible, but it would be reasonable to allow DT to set a particular
ECC strength with "nand-ecc-strength" property.  This is useful
when a particular ECC setting is hard-coded by firmware (or hard-
wired by boot ROM).

If no ECC strength is specified in DT, "nand-ecc-maximize" is implied
since this was the original behavior.

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

Changes in v3: None
Changes in v2:
  - Add available values in the binding document

 Documentation/devicetree/bindings/mtd/denali-nand.txt |  6 ++++++
 drivers/mtd/nand/denali.c                             | 18 ++++++++++++++++--
 drivers/mtd/nand/denali_pci.c                         |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 25313c7..647618e 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -11,6 +11,12 @@ 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.
+  - nand-ecc-strength: see nand.txt for details.  Available values are:
+      8, 15      for "altr,socfpga-denali-nand"
+  - nand-ecc-maximize: see nand.txt for details
+
+Note:
+Either nand-ecc-strength or nand-ecc-maximize should be specified.
 
 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 ce87b95..2f796e3 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1641,9 +1641,23 @@ int denali_init(struct denali_nand_info *denali)
 		goto failed_req_irq;
 	}
 
-	ret = denali_set_max_ecc_strength(denali);
-	if (ret)
+	if (!chip->ecc.strength && !(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
+		dev_info(denali->dev,
+			 "No ECC strength strategy is specified. Maximizing ECC strength\n");
+		chip->ecc.options |= NAND_ECC_MAXIMIZE;
+	}
+
+	if (chip->ecc.options & NAND_ECC_MAXIMIZE) {
+		ret = denali_set_max_ecc_strength(denali);
+		if (ret)
+			goto failed_req_irq;
+	} else if (!(denali->ecc_strength_avail & BIT(chip->ecc.strength))) {
+		dev_err(denali->dev,
+			"ECC strength %d is not supported on this controller.\n",
+			chip->ecc.strength);
+		ret = -EINVAL;
 		goto failed_req_irq;
+	}
 
 	chip->ecc.bytes = denali_calc_ecc_bytes(chip->ecc.size,
 						chip->ecc.strength);
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index a1ee9f8..a39682a5 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -87,6 +87,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	denali->ecc_strength_avail = BIT(15) | BIT(8);
 	denali->caps |= DENALI_CAP_ECC_SIZE_512;
+	denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
 
 	ret = denali_init(denali);
 	if (ret)
-- 
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

[PATCH v3 16/37] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Add two compatible strings for UniPhier SoCs.

"socionext,uniphier-denali-nand-v5a" is used on UniPhier sLD3, LD4,
Pro4, sLD8 SoCs.

"socionext,uniphier-denali-nand-v5b" is used on UniPhier Pro5, PXs2,
LD6b, LD11, LD20 SoCs.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3: None
Changes in v2:
  - Change the compatible strings
  - Fix the ecc_strength_capability
  - Override revision number for the newer one

 .../devicetree/bindings/mtd/denali-nand.txt        |  6 ++++++
 drivers/mtd/nand/denali_dt.c                       | 23 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 647618e..0b08ea5 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -3,6 +3,8 @@
 Required properties:
   - compatible : should be one of the following:
       "altr,socfpga-denali-nand"            - for Altera SOCFPGA
+      "socionext,uniphier-denali-nand-v5a"  - for Socionext UniPhier (v5a)
+      "socionext,uniphier-denali-nand-v5b"  - for Socionext UniPhier (v5b)
   - 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.
@@ -10,9 +12,13 @@ Required properties:
 Optional properties:
   - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to:
       512   for "altr,socfpga-denali-nand"
+      1024  for "socionext,uniphier-denali-nand-v5a"
+      1024  for "socionext,uniphier-denali-nand-v5b"
     see nand.txt for details.
   - nand-ecc-strength: see nand.txt for details.  Available values are:
       8, 15      for "altr,socfpga-denali-nand"
+      8, 16, 24  for "socionext,uniphier-denali-nand-v5a"
+      8, 16      for "socionext,uniphier-denali-nand-v5b"
   - nand-ecc-maximize: see nand.txt for details
 
 Note:
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index c3bc333..1f2f68a 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -41,11 +41,34 @@ static const struct denali_dt_data denali_socfpga_data = {
 		DENALI_CAP_ECC_SIZE_512,
 };
 
+static const struct denali_dt_data denali_uniphier_v5a_data = {
+	.ecc_strength_avail = BIT(24) | BIT(16) | BIT(8),
+	.caps = DENALI_CAP_HW_ECC_FIXUP |
+		DENALI_CAP_DMA_64BIT |
+		DENALI_CAP_ECC_SIZE_1024,
+};
+
+static const struct denali_dt_data denali_uniphier_v5b_data = {
+	.revision = 0x0501,
+	.ecc_strength_avail = BIT(16) | BIT(8),
+	.caps = DENALI_CAP_HW_ECC_FIXUP |
+		DENALI_CAP_DMA_64BIT |
+		DENALI_CAP_ECC_SIZE_1024,
+};
+
 static const struct of_device_id denali_nand_dt_ids[] = {
 	{
 		.compatible = "altr,socfpga-denali-nand",
 		.data = &denali_socfpga_data,
 	},
+	{
+		.compatible = "socionext,uniphier-denali-nand-v5a",
+		.data = &denali_uniphier_v5a_data,
+	},
+	{
+		.compatible = "socionext,uniphier-denali-nand-v5b",
+		.data = &denali_uniphier_v5b_data,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
-- 
2.7.4

Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Boris Brezillon]

On Thu, 30 Mar 2017 15:46:00 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Historically, this driver tried to choose as big ECC strength as
> possible, but it would be reasonable to allow DT to set a particular
> ECC strength with "nand-ecc-strength" property.  This is useful
> when a particular ECC setting is hard-coded by firmware (or hard-
> wired by boot ROM).
> 
> If no ECC strength is specified in DT, "nand-ecc-maximize" is implied
> since this was the original behavior.

You said there is currently no DT users, so how about changing the
"fallback to ECC maximization" behavior for DT users, and instead of
maximizing the ECC strength take the NAND requirements into account
(chip->ecc_strength_ds).

For PCI users, you explicitly set the NAND_ECC_MAXIMIZE flag, so it
shouldn't be a problem (you're still backward compatible).

> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> 
> Changes in v3: None
> Changes in v2:
>   - Add available values in the binding document
> 
>  Documentation/devicetree/bindings/mtd/denali-nand.txt |  6 ++++++
>  drivers/mtd/nand/denali.c                             | 18 ++++++++++++++++--
>  drivers/mtd/nand/denali_pci.c                         |  1 +
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 25313c7..647618e 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -11,6 +11,12 @@ 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.
> +  - nand-ecc-strength: see nand.txt for details.  Available values are:
> +      8, 15      for "altr,socfpga-denali-nand"
> +  - nand-ecc-maximize: see nand.txt for details
> +
> +Note:
> +Either nand-ecc-strength or nand-ecc-maximize should be specified.
>  
>  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 ce87b95..2f796e3 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1641,9 +1641,23 @@ int denali_init(struct denali_nand_info *denali)
>  		goto failed_req_irq;
>  	}
>  
> -	ret = denali_set_max_ecc_strength(denali);
> -	if (ret)
> +	if (!chip->ecc.strength && !(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
> +		dev_info(denali->dev,
> +			 "No ECC strength strategy is specified. Maximizing ECC strength\n");
> +		chip->ecc.options |= NAND_ECC_MAXIMIZE;
> +	}
> +
> +	if (chip->ecc.options & NAND_ECC_MAXIMIZE) {
> +		ret = denali_set_max_ecc_strength(denali);
> +		if (ret)
> +			goto failed_req_irq;
> +	} else if (!(denali->ecc_strength_avail & BIT(chip->ecc.strength))) {
> +		dev_err(denali->dev,
> +			"ECC strength %d is not supported on this controller.\n",
> +			chip->ecc.strength);
> +		ret = -EINVAL;
>  		goto failed_req_irq;
> +	}
>  
>  	chip->ecc.bytes = denali_calc_ecc_bytes(chip->ecc.size,
>  						chip->ecc.strength);
> diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
> index a1ee9f8..a39682a5 100644
> --- a/drivers/mtd/nand/denali_pci.c
> +++ b/drivers/mtd/nand/denali_pci.c
> @@ -87,6 +87,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	denali->ecc_strength_avail = BIT(15) | BIT(8);
>  	denali->caps |= DENALI_CAP_ECC_SIZE_512;
> +	denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
>  
>  	ret = denali_init(denali);
>  	if (ret)

--
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 v3 00/37] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

On Thu, 30 Mar 2017 15:45:46 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> 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.
> 
> Outstanding changes are:
> - 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
> 
> 18 patches were merged at v2.
> Here is the rest of the series.
> 
> v1: https://lkml.org/lkml/2016/11/26/144
> v2: https://lkml.org/lkml/2017/3/22/804
> 
> 
> Masahiro Yamada (37):
>   mtd: nand: relax ecc.read_page() return value for uncorrectable ECC
>   mtd: nand: denali: allow to override mtd->name from label DT property
>   mtd: nand: denali: remove meaningless pipeline read-ahead operation
>   mtd: nand: denali: fix bitflips calculation in handle_ecc()
>   mtd: nand: denali: fix erased page checking
>   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

Applied patches 1 to 11.

>   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: rework interrupt handling
>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>   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: allow drivers to request minimum alignment for passed
>     buffer
>   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

The rest looks almost good, except for a few comments I had on patch
14, 18, 25, 26 and 32.

I'll probably apply 33 and 34 soon.

> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |   24 +-
>  drivers/mtd/nand/denali.c                          | 1971 ++++++++++----------
>  drivers/mtd/nand/denali.h                          |  308 +--
>  drivers/mtd/nand/denali_dt.c                       |   90 +-
>  drivers/mtd/nand/denali_pci.c                      |   10 +-
>  drivers/mtd/nand/nand_base.c                       |   49 +-
>  include/linux/mtd/nand.h                           |    4 +-
>  7 files changed, 1234 insertions(+), 1222 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 v3 00/37] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-03-31 1:38 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:

> The rest looks almost good, except for a few comments I had on patch
> 14, 18, 25, 26 and 32.
>
> I'll probably apply 33 and 34 soon.
>

Thank you!
Please note I left a minor comment on 34.
(Accidental addition of braces.)


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,


2017-03-30 23:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 30 Mar 2017 15:46:00 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Historically, this driver tried to choose as big ECC strength as
>> possible, but it would be reasonable to allow DT to set a particular
>> ECC strength with "nand-ecc-strength" property.  This is useful
>> when a particular ECC setting is hard-coded by firmware (or hard-
>> wired by boot ROM).
>>
>> If no ECC strength is specified in DT, "nand-ecc-maximize" is implied
>> since this was the original behavior.
>
> You said there is currently no DT users,

Right.  No DT users ever in upstream.


> so how about changing the
> "fallback to ECC maximization" behavior for DT users, and instead of
> maximizing the ECC strength take the NAND requirements into account
> (chip->ecc_strength_ds).

This is difficult to judge in some cases.

As I said before, 4/512 and 8/1024 are not equivalent.

If chip's requirement  chip->ecc_step_ds matches
to the ecc->size supported by the controller,
this is easy.


If a chip requests 1024B, then the controller can only support 512B chunk
(or vice versa), it is difficult to simply compare
ecc strength.

Is it a bad thing if we use too strong ECC strength?

The disadvantage I see is we will have less OOB-free bytes,
but this will not be fatal, I guess.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 00/37] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

On Fri, 31 Mar 2017 13:05:20 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-03-31 1:38 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> 
> > The rest looks almost good, except for a few comments I had on patch
> > 14, 18, 25, 26 and 32.
> >
> > I'll probably apply 33 and 34 soon.
> >  
> 
> Thank you!
> Please note I left a minor comment on 34.
> (Accidental addition of braces.)

Yep, I noticed. Applied both after fixing the alignment and removing
unneeded braces in patch 34.

Thanks,

Boris

Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Boris Brezillon]

On Fri, 31 Mar 2017 14:06:32 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-03-30 23:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 30 Mar 2017 15:46:00 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> Historically, this driver tried to choose as big ECC strength as
> >> possible, but it would be reasonable to allow DT to set a particular
> >> ECC strength with "nand-ecc-strength" property.  This is useful
> >> when a particular ECC setting is hard-coded by firmware (or hard-
> >> wired by boot ROM).
> >>
> >> If no ECC strength is specified in DT, "nand-ecc-maximize" is implied
> >> since this was the original behavior.  
> >
> > You said there is currently no DT users,  
> 
> Right.  No DT users ever in upstream.
> 
> 
> > so how about changing the
> > "fallback to ECC maximization" behavior for DT users, and instead of
> > maximizing the ECC strength take the NAND requirements into account
> > (chip->ecc_strength_ds).  
> 
> This is difficult to judge in some cases.
> 
> As I said before, 4/512 and 8/1024 are not equivalent.
> 
> If chip's requirement  chip->ecc_step_ds matches
> to the ecc->size supported by the controller,
> this is easy.
> 
> 
> If a chip requests 1024B, then the controller can only support 512B chunk
> (or vice versa), it is difficult to simply compare
> ecc strength.

You can try something like that when no explicit ecc.strength and
ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.

static int
denali_get_closest_ecc_strength(struct denali_nand_info *denali,
				int strength)
{
	/*
	 * Whatever you need to select a strength that is greater than
	 * or equal to strength.
	 */

	return X;
}

static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
{
	struct nand_chip *chip = &denali->nand;
	struct mtd_info *mtd = nand_to_mtd(chip);
	int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
	int ecc_steps, ecc_strength, ecc_bytes;
	int ecc_size = chip->ecc_step_ds;
	int ecc_strength = chip->ecc_strength_ds;

	/*
	 * No information provided by the NAND chip, let the core
	 * maximize the strength.
	 */
	if (!ecc_size || !ecc_strength)
		return -ENOTSUPP;

	if (ecc_size > 512)
		ecc_size = 1024;
	else
		ecc_size = 512;

	/* Adjust ECC step size based on hardware support. */
	if (ecc_size == 1024 &&
	    !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
		ecc_size = 512;
	else if(ecc_size == 512 &&
		!(denali->caps & DENALI_CAP_ECC_SIZE_512))
		ecc_size = 1024;

	if (ecc_size < chip->ecc_size_ds) {
		/*
		 * When the selected size if smaller than the expected
		 * one we try to use the same strength but on 512 blocks
		 * so that we can still fix the same number of errors
		 * even if they are concentrated in the first 512bytes
		 * of a 1024bytes portion.
		 */
		ecc_strength = chip->ecc_strength_ds;
		ecc_strength = denali_get_closest_ecc_strength(denali,
							       ecc_strength);
	} else {
		/* Always prefer 1024bytes ECC blocks when possible. */
		if (ecc_size != 1024 &&
		    (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
		    mtd->writesize > 1024)
			ecc_size = 1024;

		/*
		 * Adjust the strength based on the selected ECC step
		 * size.
		 */
		ecc_strength = DIV_ROUND_UP(ecc_size,
					    chip->ecc_step_ds) *
			       chip->ecc_strength_ds;
	}

	ecc_bytes = denali_calc_ecc_bytes(ecc_size,
					  ecc_strength);
	ecc_bytes *= mtd->writesize / ecc_size;

	/*
	 * If we don't have enough space, let the core maximize
	 * the strength.
	 */
	if (ecc_bytes > max_ecc_bytes)
		return -ENOTSUPP;

	chip->ecc.strength = ecc_strength;
	chip->ecc.size = ecc_size;
	
	return 0;
}

> 
> Is it a bad thing if we use too strong ECC strength?
> 
> The disadvantage I see is we will have less OOB-free bytes,
> but this will not be fatal, I guess.

Not a bad thing in general, but I'd prefer to leave the choice to the
user. If one doesn't need the extra-safety brought by ECC strength
maximization and wants to have more OOB bytes it's better to follow
NAND requirements.

Re: [PATCH v3 12/37] mtd: nand: denali: support 1024 byte ECC step size

[Masahiro Yamada]

2017-03-30 15:45 GMT+09:00 Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>:
> 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
>
> Newer versions of this IP need ecc.size and ecc.steps explicitly
> set up via the following registers:
>   CFG_DATA_BLOCK_SIZE       (0x6b0)
>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
>   CFG_NUM_DATA_BLOCKS       (0x6d0)
>
> Older versions do not have such registers (they were 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], users can
> force ECC size via DT in case firmware hard-codes ECC settings.
> If not specified, the driver will use chip's ECC requirement as a
> hint to decide the ECC size.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>
> Changes in v3:
>   - Move DENALI_CAP_ define out of struct denali_nand_info
>   - Use chip->ecc_step_ds as a hint to choose chip->ecc.size
>     where possible
>


Please hold back this patch
until we decide how to handle 14.




-- 
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: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,



2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:

> You can try something like that when no explicit ecc.strength and
> ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
>
> static int
> denali_get_closest_ecc_strength(struct denali_nand_info *denali,
>                                 int strength)
> {
>         /*
>          * Whatever you need to select a strength that is greater than
>          * or equal to strength.
>          */
>
>         return X;
> }


Is here anything specific to Denali?


> static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
> {
>         struct nand_chip *chip = &denali->nand;
>         struct mtd_info *mtd = nand_to_mtd(chip);
>         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
>         int ecc_steps, ecc_strength, ecc_bytes;
>         int ecc_size = chip->ecc_step_ds;
>         int ecc_strength = chip->ecc_strength_ds;
>
>         /*
>          * No information provided by the NAND chip, let the core
>          * maximize the strength.
>          */
>         if (!ecc_size || !ecc_strength)
>                 return -ENOTSUPP;
>
>         if (ecc_size > 512)
>                 ecc_size = 1024;
>         else
>                 ecc_size = 512;
>
>         /* Adjust ECC step size based on hardware support. */
>         if (ecc_size == 1024 &&
>             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
>                 ecc_size = 512;
>         else if(ecc_size == 512 &&
>                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
>                 ecc_size = 1024;
>
>         if (ecc_size < chip->ecc_size_ds) {
>                 /*
>                  * When the selected size if smaller than the expected
>                  * one we try to use the same strength but on 512 blocks
>                  * so that we can still fix the same number of errors
>                  * even if they are concentrated in the first 512bytes
>                  * of a 1024bytes portion.
>                  */
>                 ecc_strength = chip->ecc_strength_ds;
>                 ecc_strength = denali_get_closest_ecc_strength(denali,
>                                                                ecc_strength);
>         } else {
>                 /* Always prefer 1024bytes ECC blocks when possible. */
>                 if (ecc_size != 1024 &&
>                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
>                     mtd->writesize > 1024)
>                         ecc_size = 1024;
>
>                 /*
>                  * Adjust the strength based on the selected ECC step
>                  * size.
>                  */
>                 ecc_strength = DIV_ROUND_UP(ecc_size,
>                                             chip->ecc_step_ds) *
>                                chip->ecc_strength_ds;
>         }
>
>         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
>                                           ecc_strength);
>         ecc_bytes *= mtd->writesize / ecc_size;
>
>         /*
>          * If we don't have enough space, let the core maximize
>          * the strength.
>          */
>         if (ecc_bytes > max_ecc_bytes)
>                 return -ENOTSUPP;
>
>         chip->ecc.strength = ecc_strength;
>         chip->ecc.size = ecc_size;
>
>         return 0;
> }


As a whole, this does not seem to driver-specific.


[1] A driver provides some pairs of (ecc_strength, ecc_size)
    it can support.

[2] The core framework knows the chip's requirement
    (ecc_strength_ds, ecc_size_ds).


Then, the core framework provides a function
to return a most recommended (ecc_strength, ecc_size).



struct nand_ecc_spec {
       int ecc_strength;
       int ecc_size;
};

/*
 * This function choose the most recommented (ecc_str, ecc_size)
 * "recommended" means: minimum ecc stregth that meets
 * the chip's requirment.
 *
 *
 * @chip   - nand_chip
 * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
                          controller. (terminated by NULL as sentinel)
 */
struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
                                                 struct nand_ecc_spec
*controller_ecc_spec)
{
      /*
       * Return the pointer to the most recommended
       * struct nand_ecc_spec.
       * If nothing suitable found, return NULL.
       */
}



Then, Denali driver can call it:


        recommended_ecc_spec = nand_try_to_match_ecc_req(chip,
                                                         denali->ecc_spec);
        if (recommended_ecc_spec) {
                chip->ecc.strength = recommended_ecc_spec.ecc_strength;
                chip->ecc.size = recommended_ecc_spec.ecc_size;
        } else {
                /*
                 * Do something  (for example, maximize the ECC)
                 */
        }





It seems weird to force this to the Denali driver.



-- 
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: [PATCH v3 16/37] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Rob Herring]

On Thu, Mar 30, 2017 at 03:46:02PM +0900, Masahiro Yamada wrote:
> Add two compatible strings for UniPhier SoCs.
> 
> "socionext,uniphier-denali-nand-v5a" is used on UniPhier sLD3, LD4,
> Pro4, sLD8 SoCs.
> 
> "socionext,uniphier-denali-nand-v5b" is used on UniPhier Pro5, PXs2,
> LD6b, LD11, LD20 SoCs.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3: None
> Changes in v2:
>   - Change the compatible strings
>   - Fix the ecc_strength_capability
>   - Override revision number for the newer one
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  6 ++++++
>  drivers/mtd/nand/denali_dt.c                       | 23 ++++++++++++++++++++++
>  2 files changed, 29 insertions(+)

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

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

Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Boris Brezillon]

On Mon, 3 Apr 2017 12:16:34 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> 
> 
> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> 
> > You can try something like that when no explicit ecc.strength and
> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
> >
> > static int
> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
> >                                 int strength)
> > {
> >         /*
> >          * Whatever you need to select a strength that is greater than
> >          * or equal to strength.
> >          */
> >
> >         return X;
> > }  
> 
> 
> Is here anything specific to Denali?

Well, only the denali driver knows what the hardware supports, though
having a generic function that takes a table of supported strengths
would work.

> 
> 
> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
> > {
> >         struct nand_chip *chip = &denali->nand;
> >         struct mtd_info *mtd = nand_to_mtd(chip);
> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
> >         int ecc_steps, ecc_strength, ecc_bytes;
> >         int ecc_size = chip->ecc_step_ds;
> >         int ecc_strength = chip->ecc_strength_ds;
> >
> >         /*
> >          * No information provided by the NAND chip, let the core
> >          * maximize the strength.
> >          */
> >         if (!ecc_size || !ecc_strength)
> >                 return -ENOTSUPP;
> >
> >         if (ecc_size > 512)
> >                 ecc_size = 1024;
> >         else
> >                 ecc_size = 512;
> >
> >         /* Adjust ECC step size based on hardware support. */
> >         if (ecc_size == 1024 &&
> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
> >                 ecc_size = 512;
> >         else if(ecc_size == 512 &&
> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
> >                 ecc_size = 1024;
> >
> >         if (ecc_size < chip->ecc_size_ds) {
> >                 /*
> >                  * When the selected size if smaller than the expected
> >                  * one we try to use the same strength but on 512 blocks
> >                  * so that we can still fix the same number of errors
> >                  * even if they are concentrated in the first 512bytes
> >                  * of a 1024bytes portion.
> >                  */
> >                 ecc_strength = chip->ecc_strength_ds;
> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
> >                                                                ecc_strength);
> >         } else {
> >                 /* Always prefer 1024bytes ECC blocks when possible. */
> >                 if (ecc_size != 1024 &&
> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
> >                     mtd->writesize > 1024)
> >                         ecc_size = 1024;
> >
> >                 /*
> >                  * Adjust the strength based on the selected ECC step
> >                  * size.
> >                  */
> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
> >                                             chip->ecc_step_ds) *
> >                                chip->ecc_strength_ds;
> >         }
> >
> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
> >                                           ecc_strength);
> >         ecc_bytes *= mtd->writesize / ecc_size;
> >
> >         /*
> >          * If we don't have enough space, let the core maximize
> >          * the strength.
> >          */
> >         if (ecc_bytes > max_ecc_bytes)
> >                 return -ENOTSUPP;
> >
> >         chip->ecc.strength = ecc_strength;
> >         chip->ecc.size = ecc_size;
> >
> >         return 0;
> > }  
> 
> 
> As a whole, this does not seem to driver-specific.

It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
function, but I guess we could ask drivers to implement a hook that is
passed the ECC step size and strength and returns the associated
number of ECC bytes.

> 
> 
> [1] A driver provides some pairs of (ecc_strength, ecc_size)
>     it can support.
> 
> [2] The core framework knows the chip's requirement
>     (ecc_strength_ds, ecc_size_ds).
> 
> 
> Then, the core framework provides a function
> to return a most recommended (ecc_strength, ecc_size).
> 
> 
> 
> struct nand_ecc_spec {
>        int ecc_strength;
>        int ecc_size;
> };
> 
> /*
>  * This function choose the most recommented (ecc_str, ecc_size)
>  * "recommended" means: minimum ecc stregth that meets
>  * the chip's requirment.
>  *
>  *
>  * @chip   - nand_chip
>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
>                           controller. (terminated by NULL as sentinel)
>  */
> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
>                                                  struct nand_ecc_spec
> *controller_ecc_spec)
> {
>       /*
>        * Return the pointer to the most recommended
>        * struct nand_ecc_spec.
>        * If nothing suitable found, return NULL.
>        */
> }
>

I like the idea, except I would do this slightly differently to avoid
declaring all combinations of stepsize and strengths

struct nand_ecc_stepsize_info {
	int stepsize;
	int nstrengths;
	int *strengths;
};

struct nand_ecc_engine_caps {
	int nstepsizes;
	struct nand_ecc_stepsize_info *stepsizes;
	int (*calc_ecc_bytes)(int stepsize, int strength);
};

int nand_try_to_match_ecc_req(struct nand_chip *chip,
			      const struct nand_ecc_engine_caps *caps,
			      struct nand_ecc_spec *spec)
{
	/*
	 * Find the most appropriate setting based on the ECC engine
	 * caps and fill the spec object accordingly.
	 * Returns 0 in case of success and a negative error code
	 * otherwise.
	 */
}

Note that nand_try_to_match_ecc_req() has to be more generic than
denali_try_to_match_ecc_req() WRT step sizes, which will probably
complexify the logic.


--
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 v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,



2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Mon, 3 Apr 2017 12:16:34 +0900
> Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
>> Hi Boris,
>>
>>
>>
>> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>>
>> > You can try something like that when no explicit ecc.strength and
>> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
>> >
>> > static int
>> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
>> >                                 int strength)
>> > {
>> >         /*
>> >          * Whatever you need to select a strength that is greater than
>> >          * or equal to strength.
>> >          */
>> >
>> >         return X;
>> > }
>>
>>
>> Is here anything specific to Denali?
>
> Well, only the denali driver knows what the hardware supports, though
> having a generic function that takes a table of supported strengths
> would work.
>
>>
>>
>> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
>> > {
>> >         struct nand_chip *chip = &denali->nand;
>> >         struct mtd_info *mtd = nand_to_mtd(chip);
>> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
>> >         int ecc_steps, ecc_strength, ecc_bytes;
>> >         int ecc_size = chip->ecc_step_ds;
>> >         int ecc_strength = chip->ecc_strength_ds;
>> >
>> >         /*
>> >          * No information provided by the NAND chip, let the core
>> >          * maximize the strength.
>> >          */
>> >         if (!ecc_size || !ecc_strength)
>> >                 return -ENOTSUPP;
>> >
>> >         if (ecc_size > 512)
>> >                 ecc_size = 1024;
>> >         else
>> >                 ecc_size = 512;
>> >
>> >         /* Adjust ECC step size based on hardware support. */
>> >         if (ecc_size == 1024 &&
>> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
>> >                 ecc_size = 512;
>> >         else if(ecc_size == 512 &&
>> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
>> >                 ecc_size = 1024;
>> >
>> >         if (ecc_size < chip->ecc_size_ds) {
>> >                 /*
>> >                  * When the selected size if smaller than the expected
>> >                  * one we try to use the same strength but on 512 blocks
>> >                  * so that we can still fix the same number of errors
>> >                  * even if they are concentrated in the first 512bytes
>> >                  * of a 1024bytes portion.
>> >                  */
>> >                 ecc_strength = chip->ecc_strength_ds;
>> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
>> >                                                                ecc_strength);
>> >         } else {
>> >                 /* Always prefer 1024bytes ECC blocks when possible. */
>> >                 if (ecc_size != 1024 &&
>> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
>> >                     mtd->writesize > 1024)
>> >                         ecc_size = 1024;
>> >
>> >                 /*
>> >                  * Adjust the strength based on the selected ECC step
>> >                  * size.
>> >                  */
>> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
>> >                                             chip->ecc_step_ds) *
>> >                                chip->ecc_strength_ds;
>> >         }
>> >
>> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
>> >                                           ecc_strength);
>> >         ecc_bytes *= mtd->writesize / ecc_size;
>> >
>> >         /*
>> >          * If we don't have enough space, let the core maximize
>> >          * the strength.
>> >          */
>> >         if (ecc_bytes > max_ecc_bytes)
>> >                 return -ENOTSUPP;
>> >
>> >         chip->ecc.strength = ecc_strength;
>> >         chip->ecc.size = ecc_size;
>> >
>> >         return 0;
>> > }
>>
>>
>> As a whole, this does not seem to driver-specific.
>
> It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
> function, but I guess we could ask drivers to implement a hook that is
> passed the ECC step size and strength and returns the associated
> number of ECC bytes.
>
>>
>>
>> [1] A driver provides some pairs of (ecc_strength, ecc_size)
>>     it can support.
>>
>> [2] The core framework knows the chip's requirement
>>     (ecc_strength_ds, ecc_size_ds).
>>
>>
>> Then, the core framework provides a function
>> to return a most recommended (ecc_strength, ecc_size).
>>
>>
>>
>> struct nand_ecc_spec {
>>        int ecc_strength;
>>        int ecc_size;
>> };
>>
>> /*
>>  * This function choose the most recommented (ecc_str, ecc_size)
>>  * "recommended" means: minimum ecc stregth that meets
>>  * the chip's requirment.
>>  *
>>  *
>>  * @chip   - nand_chip
>>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
>>                           controller. (terminated by NULL as sentinel)
>>  */
>> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
>>                                                  struct nand_ecc_spec
>> *controller_ecc_spec)
>> {
>>       /*
>>        * Return the pointer to the most recommended
>>        * struct nand_ecc_spec.
>>        * If nothing suitable found, return NULL.
>>        */
>> }
>>
>
> I like the idea, except I would do this slightly differently to avoid
> declaring all combinations of stepsize and strengths
>
> struct nand_ecc_stepsize_info {
>         int stepsize;
>         int nstrengths;
>         int *strengths;
> };
>
> struct nand_ecc_engine_caps {
>         int nstepsizes;
>         struct nand_ecc_stepsize_info *stepsizes;
>         int (*calc_ecc_bytes)(int stepsize, int strength);
> };
>
> int nand_try_to_match_ecc_req(struct nand_chip *chip,
>                               const struct nand_ecc_engine_caps *caps,
>                               struct nand_ecc_spec *spec)
> {
>         /*
>          * Find the most appropriate setting based on the ECC engine
>          * caps and fill the spec object accordingly.
>          * Returns 0 in case of success and a negative error code
>          * otherwise.
>          */
> }
>
> Note that nand_try_to_match_ecc_req() has to be more generic than
> denali_try_to_match_ecc_req() WRT step sizes, which will probably
> complexify the logic.


After I fiddle with this generic approach for a while,
I started to feel like giving up.

I wonder if we really want over-implementation
for covering _theoretically_ possible cases.

In practice, there are not so many ECC settings possible
on a single controller.

As for Denali IP, it would be theoretically possible to instantiate
multiple ECC engines.  However, in practice, there is no sensible
reason to do so.  At least, I do not know any real chip to support that.

So, I'd like to simplify the logic for Denali.

  - Support either 512 or 1024 ECC size.
    If there is (ever) a controller that supports both,
    1024 should be chosen.

  - ECC strength is not specified via DT, it is simply maximized.

This simplifies the logic much and I believe this is enough.

One more reason is, as we talked before,
we need to match ECC setting between Linux and firmware (boot-loader),
so anyway we end up with using a fixed setting specified by DT.


-- 
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: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Boris Brezillon]

Hi Masahiro,

On Tue, 11 Apr 2017 15:19:21 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> 
> 
> 2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Mon, 3 Apr 2017 12:16:34 +0900
> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >  
> >> Hi Boris,
> >>
> >>
> >>
> >> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >>  
> >> > You can try something like that when no explicit ecc.strength and
> >> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
> >> >
> >> > static int
> >> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
> >> >                                 int strength)
> >> > {
> >> >         /*
> >> >          * Whatever you need to select a strength that is greater than
> >> >          * or equal to strength.
> >> >          */
> >> >
> >> >         return X;
> >> > }  
> >>
> >>
> >> Is here anything specific to Denali?  
> >
> > Well, only the denali driver knows what the hardware supports, though
> > having a generic function that takes a table of supported strengths
> > would work.
> >  
> >>
> >>  
> >> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
> >> > {
> >> >         struct nand_chip *chip = &denali->nand;
> >> >         struct mtd_info *mtd = nand_to_mtd(chip);
> >> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
> >> >         int ecc_steps, ecc_strength, ecc_bytes;
> >> >         int ecc_size = chip->ecc_step_ds;
> >> >         int ecc_strength = chip->ecc_strength_ds;
> >> >
> >> >         /*
> >> >          * No information provided by the NAND chip, let the core
> >> >          * maximize the strength.
> >> >          */
> >> >         if (!ecc_size || !ecc_strength)
> >> >                 return -ENOTSUPP;
> >> >
> >> >         if (ecc_size > 512)
> >> >                 ecc_size = 1024;
> >> >         else
> >> >                 ecc_size = 512;
> >> >
> >> >         /* Adjust ECC step size based on hardware support. */
> >> >         if (ecc_size == 1024 &&
> >> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
> >> >                 ecc_size = 512;
> >> >         else if(ecc_size == 512 &&
> >> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
> >> >                 ecc_size = 1024;
> >> >
> >> >         if (ecc_size < chip->ecc_size_ds) {
> >> >                 /*
> >> >                  * When the selected size if smaller than the expected
> >> >                  * one we try to use the same strength but on 512 blocks
> >> >                  * so that we can still fix the same number of errors
> >> >                  * even if they are concentrated in the first 512bytes
> >> >                  * of a 1024bytes portion.
> >> >                  */
> >> >                 ecc_strength = chip->ecc_strength_ds;
> >> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
> >> >                                                                ecc_strength);
> >> >         } else {
> >> >                 /* Always prefer 1024bytes ECC blocks when possible. */
> >> >                 if (ecc_size != 1024 &&
> >> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
> >> >                     mtd->writesize > 1024)
> >> >                         ecc_size = 1024;
> >> >
> >> >                 /*
> >> >                  * Adjust the strength based on the selected ECC step
> >> >                  * size.
> >> >                  */
> >> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
> >> >                                             chip->ecc_step_ds) *
> >> >                                chip->ecc_strength_ds;
> >> >         }
> >> >
> >> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
> >> >                                           ecc_strength);
> >> >         ecc_bytes *= mtd->writesize / ecc_size;
> >> >
> >> >         /*
> >> >          * If we don't have enough space, let the core maximize
> >> >          * the strength.
> >> >          */
> >> >         if (ecc_bytes > max_ecc_bytes)
> >> >                 return -ENOTSUPP;
> >> >
> >> >         chip->ecc.strength = ecc_strength;
> >> >         chip->ecc.size = ecc_size;
> >> >
> >> >         return 0;
> >> > }  
> >>
> >>
> >> As a whole, this does not seem to driver-specific.  
> >
> > It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
> > function, but I guess we could ask drivers to implement a hook that is
> > passed the ECC step size and strength and returns the associated
> > number of ECC bytes.
> >  
> >>
> >>
> >> [1] A driver provides some pairs of (ecc_strength, ecc_size)
> >>     it can support.
> >>
> >> [2] The core framework knows the chip's requirement
> >>     (ecc_strength_ds, ecc_size_ds).
> >>
> >>
> >> Then, the core framework provides a function
> >> to return a most recommended (ecc_strength, ecc_size).
> >>
> >>
> >>
> >> struct nand_ecc_spec {
> >>        int ecc_strength;
> >>        int ecc_size;
> >> };
> >>
> >> /*
> >>  * This function choose the most recommented (ecc_str, ecc_size)
> >>  * "recommended" means: minimum ecc stregth that meets
> >>  * the chip's requirment.
> >>  *
> >>  *
> >>  * @chip   - nand_chip
> >>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
> >>                           controller. (terminated by NULL as sentinel)
> >>  */
> >> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
> >>                                                  struct nand_ecc_spec
> >> *controller_ecc_spec)
> >> {
> >>       /*
> >>        * Return the pointer to the most recommended
> >>        * struct nand_ecc_spec.
> >>        * If nothing suitable found, return NULL.
> >>        */
> >> }
> >>  
> >
> > I like the idea, except I would do this slightly differently to avoid
> > declaring all combinations of stepsize and strengths
> >
> > struct nand_ecc_stepsize_info {
> >         int stepsize;
> >         int nstrengths;
> >         int *strengths;
> > };
> >
> > struct nand_ecc_engine_caps {
> >         int nstepsizes;
> >         struct nand_ecc_stepsize_info *stepsizes;
> >         int (*calc_ecc_bytes)(int stepsize, int strength);
> > };
> >
> > int nand_try_to_match_ecc_req(struct nand_chip *chip,
> >                               const struct nand_ecc_engine_caps *caps,
> >                               struct nand_ecc_spec *spec)
> > {
> >         /*
> >          * Find the most appropriate setting based on the ECC engine
> >          * caps and fill the spec object accordingly.
> >          * Returns 0 in case of success and a negative error code
> >          * otherwise.
> >          */
> > }
> >
> > Note that nand_try_to_match_ecc_req() has to be more generic than
> > denali_try_to_match_ecc_req() WRT step sizes, which will probably
> > complexify the logic.  
> 
> 
> After I fiddle with this generic approach for a while,
> I started to feel like giving up.

I don't get it. What was the problem with my initial suggestion (the
denali specific one, not the generic approach)? You proposed to make it
generic, which, I agree, is a bit more complicated.

> 
> I wonder if we really want over-implementation
> for covering _theoretically_ possible cases.

Okay, one more theoretical case I'd like to expose: you have board
design with different NAND parts which have different ECC requirements.
If you were about to describe the exact ECC strength you want for each
board you'll have to have different DTs. Maximizing the ECC strength
would still work, but what if the MTD user needs some OOB bytes (like
is the case with JFFS2) and ECC maximization reserved all of the
available bytes?

The other reason I prefer to have the drivers automatically guessing
what's appropriate is because then you don't have to care when writing
your DT.

> 
> In practice, there are not so many ECC settings possible
> on a single controller.
> 
> As for Denali IP, it would be theoretically possible to instantiate
> multiple ECC engines.  However, in practice, there is no sensible
> reason to do so.  At least, I do not know any real chip to support that.
> 
> So, I'd like to simplify the logic for Denali.
> 
>   - Support either 512 or 1024 ECC size.
>     If there is (ever) a controller that supports both,
>     1024 should be chosen.
> 
>   - ECC strength is not specified via DT, it is simply maximized.
> 
> This simplifies the logic much and I believe this is enough.
> 
> One more reason is, as we talked before,
> we need to match ECC setting between Linux and firmware (boot-loader),

If the bootloader implements the same logic it should match.

> so anyway we end up with using a fixed setting specified by DT.
> 

Really, I don't see what's the problem with the function I proposed,
but I'm willing to make a concession.
Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize
mandatory so that if someone ever needs to support the 'match NAND
requirements' feature we won't have to add a vendor specific property
like this one [1].

Are you fine with that?

[1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/gpmi-nand.txt#L20
--
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 v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,


2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Masahiro,
>
> On Tue, 11 Apr 2017 15:19:21 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>>
>>
>> 2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Mon, 3 Apr 2017 12:16:34 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >>
>> >>
>> >> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >>
>> >> > You can try something like that when no explicit ecc.strength and
>> >> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
>> >> >
>> >> > static int
>> >> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
>> >> >                                 int strength)
>> >> > {
>> >> >         /*
>> >> >          * Whatever you need to select a strength that is greater than
>> >> >          * or equal to strength.
>> >> >          */
>> >> >
>> >> >         return X;
>> >> > }
>> >>
>> >>
>> >> Is here anything specific to Denali?
>> >
>> > Well, only the denali driver knows what the hardware supports, though
>> > having a generic function that takes a table of supported strengths
>> > would work.
>> >
>> >>
>> >>
>> >> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
>> >> > {
>> >> >         struct nand_chip *chip = &denali->nand;
>> >> >         struct mtd_info *mtd = nand_to_mtd(chip);
>> >> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
>> >> >         int ecc_steps, ecc_strength, ecc_bytes;
>> >> >         int ecc_size = chip->ecc_step_ds;
>> >> >         int ecc_strength = chip->ecc_strength_ds;
>> >> >
>> >> >         /*
>> >> >          * No information provided by the NAND chip, let the core
>> >> >          * maximize the strength.
>> >> >          */
>> >> >         if (!ecc_size || !ecc_strength)
>> >> >                 return -ENOTSUPP;
>> >> >
>> >> >         if (ecc_size > 512)
>> >> >                 ecc_size = 1024;
>> >> >         else
>> >> >                 ecc_size = 512;
>> >> >
>> >> >         /* Adjust ECC step size based on hardware support. */
>> >> >         if (ecc_size == 1024 &&
>> >> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
>> >> >                 ecc_size = 512;
>> >> >         else if(ecc_size == 512 &&
>> >> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
>> >> >                 ecc_size = 1024;
>> >> >
>> >> >         if (ecc_size < chip->ecc_size_ds) {
>> >> >                 /*
>> >> >                  * When the selected size if smaller than the expected
>> >> >                  * one we try to use the same strength but on 512 blocks
>> >> >                  * so that we can still fix the same number of errors
>> >> >                  * even if they are concentrated in the first 512bytes
>> >> >                  * of a 1024bytes portion.
>> >> >                  */
>> >> >                 ecc_strength = chip->ecc_strength_ds;
>> >> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
>> >> >                                                                ecc_strength);
>> >> >         } else {
>> >> >                 /* Always prefer 1024bytes ECC blocks when possible. */
>> >> >                 if (ecc_size != 1024 &&
>> >> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
>> >> >                     mtd->writesize > 1024)
>> >> >                         ecc_size = 1024;
>> >> >
>> >> >                 /*
>> >> >                  * Adjust the strength based on the selected ECC step
>> >> >                  * size.
>> >> >                  */
>> >> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
>> >> >                                             chip->ecc_step_ds) *
>> >> >                                chip->ecc_strength_ds;
>> >> >         }
>> >> >
>> >> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
>> >> >                                           ecc_strength);
>> >> >         ecc_bytes *= mtd->writesize / ecc_size;
>> >> >
>> >> >         /*
>> >> >          * If we don't have enough space, let the core maximize
>> >> >          * the strength.
>> >> >          */
>> >> >         if (ecc_bytes > max_ecc_bytes)
>> >> >                 return -ENOTSUPP;
>> >> >
>> >> >         chip->ecc.strength = ecc_strength;
>> >> >         chip->ecc.size = ecc_size;
>> >> >
>> >> >         return 0;
>> >> > }
>> >>
>> >>
>> >> As a whole, this does not seem to driver-specific.
>> >
>> > It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
>> > function, but I guess we could ask drivers to implement a hook that is
>> > passed the ECC step size and strength and returns the associated
>> > number of ECC bytes.
>> >
>> >>
>> >>
>> >> [1] A driver provides some pairs of (ecc_strength, ecc_size)
>> >>     it can support.
>> >>
>> >> [2] The core framework knows the chip's requirement
>> >>     (ecc_strength_ds, ecc_size_ds).
>> >>
>> >>
>> >> Then, the core framework provides a function
>> >> to return a most recommended (ecc_strength, ecc_size).
>> >>
>> >>
>> >>
>> >> struct nand_ecc_spec {
>> >>        int ecc_strength;
>> >>        int ecc_size;
>> >> };
>> >>
>> >> /*
>> >>  * This function choose the most recommented (ecc_str, ecc_size)
>> >>  * "recommended" means: minimum ecc stregth that meets
>> >>  * the chip's requirment.
>> >>  *
>> >>  *
>> >>  * @chip   - nand_chip
>> >>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
>> >>                           controller. (terminated by NULL as sentinel)
>> >>  */
>> >> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
>> >>                                                  struct nand_ecc_spec
>> >> *controller_ecc_spec)
>> >> {
>> >>       /*
>> >>        * Return the pointer to the most recommended
>> >>        * struct nand_ecc_spec.
>> >>        * If nothing suitable found, return NULL.
>> >>        */
>> >> }
>> >>
>> >
>> > I like the idea, except I would do this slightly differently to avoid
>> > declaring all combinations of stepsize and strengths
>> >
>> > struct nand_ecc_stepsize_info {
>> >         int stepsize;
>> >         int nstrengths;
>> >         int *strengths;
>> > };
>> >
>> > struct nand_ecc_engine_caps {
>> >         int nstepsizes;
>> >         struct nand_ecc_stepsize_info *stepsizes;
>> >         int (*calc_ecc_bytes)(int stepsize, int strength);
>> > };
>> >
>> > int nand_try_to_match_ecc_req(struct nand_chip *chip,
>> >                               const struct nand_ecc_engine_caps *caps,
>> >                               struct nand_ecc_spec *spec)
>> > {
>> >         /*
>> >          * Find the most appropriate setting based on the ECC engine
>> >          * caps and fill the spec object accordingly.
>> >          * Returns 0 in case of success and a negative error code
>> >          * otherwise.
>> >          */
>> > }
>> >
>> > Note that nand_try_to_match_ecc_req() has to be more generic than
>> > denali_try_to_match_ecc_req() WRT step sizes, which will probably
>> > complexify the logic.
>>
>>
>> After I fiddle with this generic approach for a while,
>> I started to feel like giving up.
>
> I don't get it. What was the problem with my initial suggestion (the
> denali specific one, not the generic approach)? You proposed to make it
> generic, which, I agree, is a bit more complicated.
>
>>
>> I wonder if we really want over-implementation
>> for covering _theoretically_ possible cases.
>
> Okay, one more theoretical case I'd like to expose: you have board
> design with different NAND parts which have different ECC requirements.
> If you were about to describe the exact ECC strength you want for each
> board you'll have to have different DTs.

In this case, fixed ecc-strength in DT is not feasible.

> Maximizing the ECC strength
> would still work, but what if the MTD user needs some OOB bytes (like
> is the case with JFFS2) and ECC maximization reserved all of the
> available bytes?

JFFS2 needs some bytes in oob-free area for the clean marker.
You are right.
This implies NAND_ECC_MAXIMIZE is not very useful.
We do not know whether we have enough space left in oob, or not.



> The other reason I prefer to have the drivers automatically guessing
> what's appropriate is because then you don't have to care when writing
> your DT.
>
>>
>> In practice, there are not so many ECC settings possible
>> on a single controller.
>>
>> As for Denali IP, it would be theoretically possible to instantiate
>> multiple ECC engines.  However, in practice, there is no sensible
>> reason to do so.  At least, I do not know any real chip to support that.
>>
>> So, I'd like to simplify the logic for Denali.
>>
>>   - Support either 512 or 1024 ECC size.
>>     If there is (ever) a controller that supports both,
>>     1024 should be chosen.
>>
>>   - ECC strength is not specified via DT, it is simply maximized.
>>
>> This simplifies the logic much and I believe this is enough.
>>
>> One more reason is, as we talked before,
>> we need to match ECC setting between Linux and firmware (boot-loader),
>
> If the bootloader implements the same logic it should match.
>
>> so anyway we end up with using a fixed setting specified by DT.
>>
>
> Really, I don't see what's the problem with the function I proposed,
> but I'm willing to make a concession.
> Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize
> mandatory so that if someone ever needs to support the 'match NAND
> requirements' feature we won't have to add a vendor specific property
> like this one [1].
>
> Are you fine with that?

No.  This requirement seems too strong.
At least, it is a problem for non-DT platforms.


If a driver provides ECC engine caps info,
perhaps ECC maximizing could be a generalized helper function as well.

I am trying this still.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Boris Brezillon]

On Fri, 14 Apr 2017 16:57:23 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> 
> 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > Hi Masahiro,
> >
> > On Tue, 11 Apr 2017 15:19:21 +0900
> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >  
> >> Hi Boris,
> >>
> >>
> >>
> >> 2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:  
> >> > On Mon, 3 Apr 2017 12:16:34 +0900
> >> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >> >  
> >> >> Hi Boris,
> >> >>
> >> >>
> >> >>
> >> >> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >> >>  
> >> >> > You can try something like that when no explicit ecc.strength and
> >> >> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
> >> >> >
> >> >> > static int
> >> >> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
> >> >> >                                 int strength)
> >> >> > {
> >> >> >         /*
> >> >> >          * Whatever you need to select a strength that is greater than
> >> >> >          * or equal to strength.
> >> >> >          */
> >> >> >
> >> >> >         return X;
> >> >> > }  
> >> >>
> >> >>
> >> >> Is here anything specific to Denali?  
> >> >
> >> > Well, only the denali driver knows what the hardware supports, though
> >> > having a generic function that takes a table of supported strengths
> >> > would work.
> >> >  
> >> >>
> >> >>  
> >> >> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
> >> >> > {
> >> >> >         struct nand_chip *chip = &denali->nand;
> >> >> >         struct mtd_info *mtd = nand_to_mtd(chip);
> >> >> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
> >> >> >         int ecc_steps, ecc_strength, ecc_bytes;
> >> >> >         int ecc_size = chip->ecc_step_ds;
> >> >> >         int ecc_strength = chip->ecc_strength_ds;
> >> >> >
> >> >> >         /*
> >> >> >          * No information provided by the NAND chip, let the core
> >> >> >          * maximize the strength.
> >> >> >          */
> >> >> >         if (!ecc_size || !ecc_strength)
> >> >> >                 return -ENOTSUPP;
> >> >> >
> >> >> >         if (ecc_size > 512)
> >> >> >                 ecc_size = 1024;
> >> >> >         else
> >> >> >                 ecc_size = 512;
> >> >> >
> >> >> >         /* Adjust ECC step size based on hardware support. */
> >> >> >         if (ecc_size == 1024 &&
> >> >> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
> >> >> >                 ecc_size = 512;
> >> >> >         else if(ecc_size == 512 &&
> >> >> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
> >> >> >                 ecc_size = 1024;
> >> >> >
> >> >> >         if (ecc_size < chip->ecc_size_ds) {
> >> >> >                 /*
> >> >> >                  * When the selected size if smaller than the expected
> >> >> >                  * one we try to use the same strength but on 512 blocks
> >> >> >                  * so that we can still fix the same number of errors
> >> >> >                  * even if they are concentrated in the first 512bytes
> >> >> >                  * of a 1024bytes portion.
> >> >> >                  */
> >> >> >                 ecc_strength = chip->ecc_strength_ds;
> >> >> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
> >> >> >                                                                ecc_strength);
> >> >> >         } else {
> >> >> >                 /* Always prefer 1024bytes ECC blocks when possible. */
> >> >> >                 if (ecc_size != 1024 &&
> >> >> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
> >> >> >                     mtd->writesize > 1024)
> >> >> >                         ecc_size = 1024;
> >> >> >
> >> >> >                 /*
> >> >> >                  * Adjust the strength based on the selected ECC step
> >> >> >                  * size.
> >> >> >                  */
> >> >> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
> >> >> >                                             chip->ecc_step_ds) *
> >> >> >                                chip->ecc_strength_ds;
> >> >> >         }
> >> >> >
> >> >> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
> >> >> >                                           ecc_strength);
> >> >> >         ecc_bytes *= mtd->writesize / ecc_size;
> >> >> >
> >> >> >         /*
> >> >> >          * If we don't have enough space, let the core maximize
> >> >> >          * the strength.
> >> >> >          */
> >> >> >         if (ecc_bytes > max_ecc_bytes)
> >> >> >                 return -ENOTSUPP;
> >> >> >
> >> >> >         chip->ecc.strength = ecc_strength;
> >> >> >         chip->ecc.size = ecc_size;
> >> >> >
> >> >> >         return 0;
> >> >> > }  
> >> >>
> >> >>
> >> >> As a whole, this does not seem to driver-specific.  
> >> >
> >> > It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
> >> > function, but I guess we could ask drivers to implement a hook that is
> >> > passed the ECC step size and strength and returns the associated
> >> > number of ECC bytes.
> >> >  
> >> >>
> >> >>
> >> >> [1] A driver provides some pairs of (ecc_strength, ecc_size)
> >> >>     it can support.
> >> >>
> >> >> [2] The core framework knows the chip's requirement
> >> >>     (ecc_strength_ds, ecc_size_ds).
> >> >>
> >> >>
> >> >> Then, the core framework provides a function
> >> >> to return a most recommended (ecc_strength, ecc_size).
> >> >>
> >> >>
> >> >>
> >> >> struct nand_ecc_spec {
> >> >>        int ecc_strength;
> >> >>        int ecc_size;
> >> >> };
> >> >>
> >> >> /*
> >> >>  * This function choose the most recommented (ecc_str, ecc_size)
> >> >>  * "recommended" means: minimum ecc stregth that meets
> >> >>  * the chip's requirment.
> >> >>  *
> >> >>  *
> >> >>  * @chip   - nand_chip
> >> >>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
> >> >>                           controller. (terminated by NULL as sentinel)
> >> >>  */
> >> >> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
> >> >>                                                  struct nand_ecc_spec
> >> >> *controller_ecc_spec)
> >> >> {
> >> >>       /*
> >> >>        * Return the pointer to the most recommended
> >> >>        * struct nand_ecc_spec.
> >> >>        * If nothing suitable found, return NULL.
> >> >>        */
> >> >> }
> >> >>  
> >> >
> >> > I like the idea, except I would do this slightly differently to avoid
> >> > declaring all combinations of stepsize and strengths
> >> >
> >> > struct nand_ecc_stepsize_info {
> >> >         int stepsize;
> >> >         int nstrengths;
> >> >         int *strengths;
> >> > };
> >> >
> >> > struct nand_ecc_engine_caps {
> >> >         int nstepsizes;
> >> >         struct nand_ecc_stepsize_info *stepsizes;
> >> >         int (*calc_ecc_bytes)(int stepsize, int strength);
> >> > };
> >> >
> >> > int nand_try_to_match_ecc_req(struct nand_chip *chip,
> >> >                               const struct nand_ecc_engine_caps *caps,
> >> >                               struct nand_ecc_spec *spec)
> >> > {
> >> >         /*
> >> >          * Find the most appropriate setting based on the ECC engine
> >> >          * caps and fill the spec object accordingly.
> >> >          * Returns 0 in case of success and a negative error code
> >> >          * otherwise.
> >> >          */
> >> > }
> >> >
> >> > Note that nand_try_to_match_ecc_req() has to be more generic than
> >> > denali_try_to_match_ecc_req() WRT step sizes, which will probably
> >> > complexify the logic.  
> >>
> >>
> >> After I fiddle with this generic approach for a while,
> >> I started to feel like giving up.  
> >
> > I don't get it. What was the problem with my initial suggestion (the
> > denali specific one, not the generic approach)? You proposed to make it
> > generic, which, I agree, is a bit more complicated.
> >  
> >>
> >> I wonder if we really want over-implementation
> >> for covering _theoretically_ possible cases.  
> >
> > Okay, one more theoretical case I'd like to expose: you have board
> > design with different NAND parts which have different ECC requirements.
> > If you were about to describe the exact ECC strength you want for each
> > board you'll have to have different DTs.  
> 
> In this case, fixed ecc-strength in DT is not feasible.
> 
> > Maximizing the ECC strength
> > would still work, but what if the MTD user needs some OOB bytes (like
> > is the case with JFFS2) and ECC maximization reserved all of the
> > available bytes?  
> 
> JFFS2 needs some bytes in oob-free area for the clean marker.
> You are right.
> This implies NAND_ECC_MAXIMIZE is not very useful.
> We do not know whether we have enough space left in oob, or not.
> 
> 
> 
> > The other reason I prefer to have the drivers automatically guessing
> > what's appropriate is because then you don't have to care when writing
> > your DT.
> >  
> >>
> >> In practice, there are not so many ECC settings possible
> >> on a single controller.
> >>
> >> As for Denali IP, it would be theoretically possible to instantiate
> >> multiple ECC engines.  However, in practice, there is no sensible
> >> reason to do so.  At least, I do not know any real chip to support that.
> >>
> >> So, I'd like to simplify the logic for Denali.
> >>
> >>   - Support either 512 or 1024 ECC size.
> >>     If there is (ever) a controller that supports both,
> >>     1024 should be chosen.
> >>
> >>   - ECC strength is not specified via DT, it is simply maximized.
> >>
> >> This simplifies the logic much and I believe this is enough.
> >>
> >> One more reason is, as we talked before,
> >> we need to match ECC setting between Linux and firmware (boot-loader),  
> >
> > If the bootloader implements the same logic it should match.
> >  
> >> so anyway we end up with using a fixed setting specified by DT.
> >>  
> >
> > Really, I don't see what's the problem with the function I proposed,
> > but I'm willing to make a concession.
> > Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize
> > mandatory so that if someone ever needs to support the 'match NAND
> > requirements' feature we won't have to add a vendor specific property
> > like this one [1].
> >
> > Are you fine with that?  
> 
> No.  This requirement seems too strong.

Hm, can you give more details? All I want is a solution where we can
later support the feature I'm asking without adding a extra DT
property, and, in order to do that we must make sure the case you want
to support as a first step are explicitly requested in the DT.

It's as simple as:

	if ((!ecc->strength || !ecc->size) &&
	    !(ecc->options & NAND_ECC_MAXIMIZE))
		return -ENOTSUPP;

> At least, it is a problem for non-DT platforms.

Well, for non-DT platforms you have to keep ECC maximization anyway,
otherwise you're not backward compatible.

> 
> 
> If a driver provides ECC engine caps info,
> perhaps ECC maximizing could be a generalized helper function as well.

I don't get it. I thought the generic helper was too hard to implement.
Now you want to add a new functionality.

I'm not against this idea, but maybe it's easier to provide a denali
specific implementation before tackling the generic one.
--
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 v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,


2017-04-14 17:19 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Fri, 14 Apr 2017 16:57:23 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>>
>> 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > Hi Masahiro,
>> >
>> > On Tue, 11 Apr 2017 15:19:21 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >>
>> >>
>> >> 2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > On Mon, 3 Apr 2017 12:16:34 +0900
>> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >
>> >> >> Hi Boris,
>> >> >>
>> >> >>
>> >> >>
>> >> >> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> >>
>> >> >> > You can try something like that when no explicit ecc.strength and
>> >> >> > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
>> >> >> >
>> >> >> > static int
>> >> >> > denali_get_closest_ecc_strength(struct denali_nand_info *denali,
>> >> >> >                                 int strength)
>> >> >> > {
>> >> >> >         /*
>> >> >> >          * Whatever you need to select a strength that is greater than
>> >> >> >          * or equal to strength.
>> >> >> >          */
>> >> >> >
>> >> >> >         return X;
>> >> >> > }
>> >> >>
>> >> >>
>> >> >> Is here anything specific to Denali?
>> >> >
>> >> > Well, only the denali driver knows what the hardware supports, though
>> >> > having a generic function that takes a table of supported strengths
>> >> > would work.
>> >> >
>> >> >>
>> >> >>
>> >> >> > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
>> >> >> > {
>> >> >> >         struct nand_chip *chip = &denali->nand;
>> >> >> >         struct mtd_info *mtd = nand_to_mtd(chip);
>> >> >> >         int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
>> >> >> >         int ecc_steps, ecc_strength, ecc_bytes;
>> >> >> >         int ecc_size = chip->ecc_step_ds;
>> >> >> >         int ecc_strength = chip->ecc_strength_ds;
>> >> >> >
>> >> >> >         /*
>> >> >> >          * No information provided by the NAND chip, let the core
>> >> >> >          * maximize the strength.
>> >> >> >          */
>> >> >> >         if (!ecc_size || !ecc_strength)
>> >> >> >                 return -ENOTSUPP;
>> >> >> >
>> >> >> >         if (ecc_size > 512)
>> >> >> >                 ecc_size = 1024;
>> >> >> >         else
>> >> >> >                 ecc_size = 512;
>> >> >> >
>> >> >> >         /* Adjust ECC step size based on hardware support. */
>> >> >> >         if (ecc_size == 1024 &&
>> >> >> >             !(denali->caps & DENALI_CAP_ECC_SIZE_1024))
>> >> >> >                 ecc_size = 512;
>> >> >> >         else if(ecc_size == 512 &&
>> >> >> >                 !(denali->caps & DENALI_CAP_ECC_SIZE_512))
>> >> >> >                 ecc_size = 1024;
>> >> >> >
>> >> >> >         if (ecc_size < chip->ecc_size_ds) {
>> >> >> >                 /*
>> >> >> >                  * When the selected size if smaller than the expected
>> >> >> >                  * one we try to use the same strength but on 512 blocks
>> >> >> >                  * so that we can still fix the same number of errors
>> >> >> >                  * even if they are concentrated in the first 512bytes
>> >> >> >                  * of a 1024bytes portion.
>> >> >> >                  */
>> >> >> >                 ecc_strength = chip->ecc_strength_ds;
>> >> >> >                 ecc_strength = denali_get_closest_ecc_strength(denali,
>> >> >> >                                                                ecc_strength);
>> >> >> >         } else {
>> >> >> >                 /* Always prefer 1024bytes ECC blocks when possible. */
>> >> >> >                 if (ecc_size != 1024 &&
>> >> >> >                     (denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
>> >> >> >                     mtd->writesize > 1024)
>> >> >> >                         ecc_size = 1024;
>> >> >> >
>> >> >> >                 /*
>> >> >> >                  * Adjust the strength based on the selected ECC step
>> >> >> >                  * size.
>> >> >> >                  */
>> >> >> >                 ecc_strength = DIV_ROUND_UP(ecc_size,
>> >> >> >                                             chip->ecc_step_ds) *
>> >> >> >                                chip->ecc_strength_ds;
>> >> >> >         }
>> >> >> >
>> >> >> >         ecc_bytes = denali_calc_ecc_bytes(ecc_size,
>> >> >> >                                           ecc_strength);
>> >> >> >         ecc_bytes *= mtd->writesize / ecc_size;
>> >> >> >
>> >> >> >         /*
>> >> >> >          * If we don't have enough space, let the core maximize
>> >> >> >          * the strength.
>> >> >> >          */
>> >> >> >         if (ecc_bytes > max_ecc_bytes)
>> >> >> >                 return -ENOTSUPP;
>> >> >> >
>> >> >> >         chip->ecc.strength = ecc_strength;
>> >> >> >         chip->ecc.size = ecc_size;
>> >> >> >
>> >> >> >         return 0;
>> >> >> > }
>> >> >>
>> >> >>
>> >> >> As a whole, this does not seem to driver-specific.
>> >> >
>> >> > It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
>> >> > function, but I guess we could ask drivers to implement a hook that is
>> >> > passed the ECC step size and strength and returns the associated
>> >> > number of ECC bytes.
>> >> >
>> >> >>
>> >> >>
>> >> >> [1] A driver provides some pairs of (ecc_strength, ecc_size)
>> >> >>     it can support.
>> >> >>
>> >> >> [2] The core framework knows the chip's requirement
>> >> >>     (ecc_strength_ds, ecc_size_ds).
>> >> >>
>> >> >>
>> >> >> Then, the core framework provides a function
>> >> >> to return a most recommended (ecc_strength, ecc_size).
>> >> >>
>> >> >>
>> >> >>
>> >> >> struct nand_ecc_spec {
>> >> >>        int ecc_strength;
>> >> >>        int ecc_size;
>> >> >> };
>> >> >>
>> >> >> /*
>> >> >>  * This function choose the most recommented (ecc_str, ecc_size)
>> >> >>  * "recommended" means: minimum ecc stregth that meets
>> >> >>  * the chip's requirment.
>> >> >>  *
>> >> >>  *
>> >> >>  * @chip   - nand_chip
>> >> >>  * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
>> >> >>                           controller. (terminated by NULL as sentinel)
>> >> >>  */
>> >> >> struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
>> >> >>                                                  struct nand_ecc_spec
>> >> >> *controller_ecc_spec)
>> >> >> {
>> >> >>       /*
>> >> >>        * Return the pointer to the most recommended
>> >> >>        * struct nand_ecc_spec.
>> >> >>        * If nothing suitable found, return NULL.
>> >> >>        */
>> >> >> }
>> >> >>
>> >> >
>> >> > I like the idea, except I would do this slightly differently to avoid
>> >> > declaring all combinations of stepsize and strengths
>> >> >
>> >> > struct nand_ecc_stepsize_info {
>> >> >         int stepsize;
>> >> >         int nstrengths;
>> >> >         int *strengths;
>> >> > };
>> >> >
>> >> > struct nand_ecc_engine_caps {
>> >> >         int nstepsizes;
>> >> >         struct nand_ecc_stepsize_info *stepsizes;
>> >> >         int (*calc_ecc_bytes)(int stepsize, int strength);
>> >> > };
>> >> >
>> >> > int nand_try_to_match_ecc_req(struct nand_chip *chip,
>> >> >                               const struct nand_ecc_engine_caps *caps,
>> >> >                               struct nand_ecc_spec *spec)
>> >> > {
>> >> >         /*
>> >> >          * Find the most appropriate setting based on the ECC engine
>> >> >          * caps and fill the spec object accordingly.
>> >> >          * Returns 0 in case of success and a negative error code
>> >> >          * otherwise.
>> >> >          */
>> >> > }
>> >> >
>> >> > Note that nand_try_to_match_ecc_req() has to be more generic than
>> >> > denali_try_to_match_ecc_req() WRT step sizes, which will probably
>> >> > complexify the logic.
>> >>
>> >>
>> >> After I fiddle with this generic approach for a while,
>> >> I started to feel like giving up.
>> >
>> > I don't get it. What was the problem with my initial suggestion (the
>> > denali specific one, not the generic approach)? You proposed to make it
>> > generic, which, I agree, is a bit more complicated.
>> >
>> >>
>> >> I wonder if we really want over-implementation
>> >> for covering _theoretically_ possible cases.
>> >
>> > Okay, one more theoretical case I'd like to expose: you have board
>> > design with different NAND parts which have different ECC requirements.
>> > If you were about to describe the exact ECC strength you want for each
>> > board you'll have to have different DTs.
>>
>> In this case, fixed ecc-strength in DT is not feasible.
>>
>> > Maximizing the ECC strength
>> > would still work, but what if the MTD user needs some OOB bytes (like
>> > is the case with JFFS2) and ECC maximization reserved all of the
>> > available bytes?
>>
>> JFFS2 needs some bytes in oob-free area for the clean marker.
>> You are right.
>> This implies NAND_ECC_MAXIMIZE is not very useful.
>> We do not know whether we have enough space left in oob, or not.
>>
>>
>>
>> > The other reason I prefer to have the drivers automatically guessing
>> > what's appropriate is because then you don't have to care when writing
>> > your DT.
>> >
>> >>
>> >> In practice, there are not so many ECC settings possible
>> >> on a single controller.
>> >>
>> >> As for Denali IP, it would be theoretically possible to instantiate
>> >> multiple ECC engines.  However, in practice, there is no sensible
>> >> reason to do so.  At least, I do not know any real chip to support that.
>> >>
>> >> So, I'd like to simplify the logic for Denali.
>> >>
>> >>   - Support either 512 or 1024 ECC size.
>> >>     If there is (ever) a controller that supports both,
>> >>     1024 should be chosen.
>> >>
>> >>   - ECC strength is not specified via DT, it is simply maximized.
>> >>
>> >> This simplifies the logic much and I believe this is enough.
>> >>
>> >> One more reason is, as we talked before,
>> >> we need to match ECC setting between Linux and firmware (boot-loader),
>> >
>> > If the bootloader implements the same logic it should match.
>> >
>> >> so anyway we end up with using a fixed setting specified by DT.
>> >>
>> >
>> > Really, I don't see what's the problem with the function I proposed,
>> > but I'm willing to make a concession.
>> > Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize
>> > mandatory so that if someone ever needs to support the 'match NAND
>> > requirements' feature we won't have to add a vendor specific property
>> > like this one [1].
>> >
>> > Are you fine with that?
>>
>> No.  This requirement seems too strong.
>
> Hm, can you give more details? All I want is a solution where we can
> later support the feature I'm asking without adding a extra DT
> property, and, in order to do that we must make sure the case you want
> to support as a first step are explicitly requested in the DT.
>
> It's as simple as:
>
>         if ((!ecc->strength || !ecc->size) &&
>             !(ecc->options & NAND_ECC_MAXIMIZE))
>                 return -ENOTSUPP;

If a controller supports only one possible value for nand-ecc-step-size,
users have no choice anyway.

For UniPhier SoCs,
    nand-ecc-step-size = <1024>;
    nand-ecc-strength = <8> or <16> or <24>;

But, it is harmless even if we specify nand-ecc-step-size explicitly.
So, I do not argue here.



>> At least, it is a problem for non-DT platforms.
>
> Well, for non-DT platforms you have to keep ECC maximization anyway,
> otherwise you're not backward compatible.
>
>>
>>
>> If a driver provides ECC engine caps info,
>> perhaps ECC maximizing could be a generalized helper function as well.
>
> I don't get it. I thought the generic helper was too hard to implement.
> Now you want to add a new functionality.
>
> I'm not against this idea, but maybe it's easier to provide a denali
> specific implementation before tackling the generic one.


I think there is a common logic in matching request and maximizing.

I could not explain well in my words, so I wrote a patch:
http://patchwork.ozlabs.org/patch/752107/

Could you check it?



-- 
Best Regards
Masahiro Yamada

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