[PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb

[PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

This patch series intends to solve various 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

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.
11 patches were merged at v3.
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
v3: https://lkml.org/lkml/2017/3/30/90


Masahiro Yamada (23):
  mtd: nand: denali_dt: clean up resource ioremap
  mtd: nand: denali: use BIT() and GENMASK() for register macros
  mtd: nand: add generic helpers to check, match, maximize ECC settings
  mtd: nand: denali: avoid hard-coding ECC step, strength, bytes
  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: 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 to detect the number of
    chips
  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: 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        |   13 +
 drivers/mtd/nand/denali.c                          | 1701 +++++++++-----------
 drivers/mtd/nand/denali.h                          |  294 ++--
 drivers/mtd/nand/denali_dt.c                       |   54 +-
 drivers/mtd/nand/denali_pci.c                      |   15 +-
 drivers/mtd/nand/nand_base.c                       |  219 +++
 include/linux/mtd/nand.h                           |   35 +
 7 files changed, 1235 insertions(+), 1096 deletions(-)

-- 
2.7.4

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

[PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Masahiro Yamada]

This driver was originally written for the Intel MRST platform with
several platform-specific parameters hard-coded.

Currently, the ECC settings are hard-coded as follows:

  #define ECC_SECTOR_SIZE 512
  #define ECC_8BITS       14
  #define ECC_15BITS      26

Therefore, the driver can only support two cases.
 - ecc.size = 512, ecc.strength = 8    --> ecc.bytes = 14
 - ecc.size = 512, ecc.strength = 15   --> ecc.bytes = 26

However, these are actually customizable parameters, for example,
UniPhier platform supports the following:

 - ecc.size = 1024, ecc.strength = 8   --> ecc.bytes = 14
 - ecc.size = 1024, ecc.strength = 16  --> ecc.bytes = 28
 - ecc.size = 1024, ecc.strength = 24  --> ecc.bytes = 42

So, we need to handle the ECC parameters in a more generic manner.
Fortunately, the Denali User's Guide explains how to calculate the
ecc.bytes.  The formula is:

  ecc.bytes = 2 * CEIL(13 * ecc.strength / 16)  (for ecc.size = 512)
  ecc.bytes = 2 * CEIL(14 * ecc.strength / 16)  (for ecc.size = 1024)

For DT platforms, it would be reasonable to allow DT to specify ECC
strength by either "nand-ecc-strength" or "nand-ecc-maximize".  If
none of them is specified, the driver will try to meet the chip's ECC
requirement.

For PCI platforms, the max ECC strength is used to keep the original
behavior.

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)

For older IP versions, write accesses to these registers are just
ignored.

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

Changes in v4:
  - Rewrite by using generic helpers, nand_check_caps(),
    nand_match_ecc_req(), nand_maximize_ecc().

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        |   7 ++
 drivers/mtd/nand/denali.c                          | 103 ++++++++++++++-------
 drivers/mtd/nand/denali.h                          |  11 ++-
 drivers/mtd/nand/denali_dt.c                       |   8 ++
 drivers/mtd/nand/denali_pci.c                      |   9 ++
 5 files changed, 101 insertions(+), 37 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index e593bbe..b7742a7 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -7,6 +7,13 @@ Required properties:
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
 
+Optional properties:
+  - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
+      512        for "altr,socfpga-denali-nand"
+  - nand-ecc-strength: see nand.txt for details.  Valid values are:
+      8, 15      for "altr,socfpga-denali-nand"
+  - nand-ecc-maximize: 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..3204c51 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 */
@@ -1345,13 +1344,55 @@ static void denali_hw_init(struct denali_nand_info *denali)
 	denali_irq_init(denali);
 }
 
-/*
- * Althogh controller spec said SLC ECC is forceb to be 4bit,
- * but denali controller in MRST only support 15bit and 8bit ECC
- * correction
- */
-#define ECC_8BITS	14
-#define ECC_15BITS	26
+static int denali_calc_ecc_bytes(int step_size, int strength)
+{
+	int coef;
+
+	switch (step_size) {
+	case 512:
+		coef = 13;
+		break;
+	case 1024:
+		coef = 14;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return DIV_ROUND_UP(strength * coef, 16) * 2;
+}
+
+static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
+			    struct denali_nand_info *denali)
+{
+	struct nand_ecc_caps caps;
+	int ret;
+
+	caps.stepinfos = denali->stepinfo;
+	caps.nstepinfos = 1;
+	caps.calc_ecc_bytes = denali_calc_ecc_bytes;
+	caps.oob_reserve_bytes = denali->bbtskipbytes;
+
+	/*
+	 * If .size and .strength are already set (usually by DT),
+	 * check if they are supported by this controller.
+	 */
+	if (chip->ecc.size && chip->ecc.strength)
+		return nand_check_ecc_caps(mtd, chip, &caps);
+
+	/*
+	 * We want .size and .strength closest to the chip's requirement
+	 * unless NAND_ECC_MAXIMIZE is requested.
+	 */
+	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
+		ret = nand_match_ecc_req(mtd, chip, &caps);
+		if (!ret)
+			return 0;
+	}
+
+	/* Max ECC strength is the last thing we can do */
+	return nand_maximize_ecc(mtd, chip, &caps);
+}
 
 static int denali_ooblayout_ecc(struct mtd_info *mtd, int section,
 				struct mtd_oob_region *oobregion)
@@ -1586,34 +1627,26 @@ int denali_init(struct denali_nand_info *denali)
 	/* no subpage writes on denali */
 	chip->options |= NAND_NO_SUBPAGE_WRITE;
 
-	/*
-	 * 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)))) {
-		/* 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))) {
-		pr_err("Your NAND chip OOB is not large enough to contain 8bit ECC correction codes");
+	ret = denali_ecc_setup(mtd, chip, denali);
+	if (ret) {
+		dev_err(denali->dev, "Failed to setup ECC settings.\n");
 		goto failed_req_irq;
-	} else {
-		chip->ecc.strength = 8;
-		chip->ecc.bytes = ECC_8BITS;
-		iowrite32(8, denali->flash_reg + ECC_CORRECTION);
 	}
 
+	dev_dbg(denali->dev,
+		"chosen ECC settings: step=%d, strength=%d, bytes=%d\n",
+		chip->ecc.size, chip->ecc.strength, chip->ecc.bytes);
+
+	iowrite32(chip->ecc.strength, 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 3783353..5f08691 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -259,6 +259,14 @@
 #define     ECC_COR_INFO__MAX_ERRORS			GENMASK(6, 0)
 #define     ECC_COR_INFO__UNCOR_ERR			BIT(7)
 
+#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				BIT(0)
 
@@ -301,8 +309,6 @@
 #define MODE_10    0x08000000
 #define MODE_11    0x0C000000
 
-#define ECC_SECTOR_SIZE     512
-
 struct nand_buf {
 	int head;
 	int tail;
@@ -337,6 +343,7 @@ struct denali_nand_info {
 	int max_banks;
 	unsigned int revision;
 	unsigned int caps;
+	const struct nand_ecc_step_info *stepinfo;
 };
 
 #define DENALI_CAP_HW_ECC_FIXUP			BIT(0)
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index b48430f..8c09bbe 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -32,10 +32,17 @@ struct denali_dt {
 struct denali_dt_data {
 	unsigned int revision;
 	unsigned int caps;
+	struct nand_ecc_step_info stepinfo;
 };
 
+static const int denali_socfpga_strengths[] = {8, 15};
 static const struct denali_dt_data denali_socfpga_data = {
 	.caps = DENALI_CAP_HW_ECC_FIXUP,
+	.stepinfo = {
+		.stepsize = 512,
+		.strengths = denali_socfpga_strengths,
+		.nstrengths = ARRAY_SIZE(denali_socfpga_strengths),
+	},
 };
 
 static const struct of_device_id denali_nand_dt_ids[] = {
@@ -64,6 +71,7 @@ static int denali_dt_probe(struct platform_device *pdev)
 	if (data) {
 		denali->revision = data->revision;
 		denali->caps = data->caps;
+		denali->stepinfo = &data->stepinfo;
 	}
 
 	denali->platform = DT;
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index ac84323..e0d50b6 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -27,6 +27,13 @@ static const struct pci_device_id denali_pci_ids[] = {
 };
 MODULE_DEVICE_TABLE(pci, denali_pci_ids);
 
+static const int denali_pci_strengths[] = {8, 15};
+static const struct nand_ecc_step_info denali_pci_stepinfo = {
+	.stepsize = 512,
+	.strengths = denali_pci_strengths,
+	.nstrengths = ARRAY_SIZE(denali_pci_strengths),
+};
+
 static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int ret;
@@ -65,6 +72,8 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	pci_set_master(dev);
 	denali->dev = &dev->dev;
 	denali->irq = dev->irq;
+	denali->stepinfo = &denali_pci_stepinfo;
+	denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
 
 	ret = pci_request_regions(dev, DENALI_NAND_NAME);
 	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 v4 06/23] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Add two compatible strings for UniPhier SoC family.

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

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

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

Changes in v4:
  - Adjusted to generic helpers for ECC engine caps

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                       | 31 ++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index b7742a7..504291d 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,8 +12,12 @@ Required properties:
 Optional properties:
   - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
       512        for "altr,socfpga-denali-nand"
+      1024       for "socionext,uniphier-denali-nand-v5a"
+      1024       for "socionext,uniphier-denali-nand-v5b"
   - nand-ecc-strength: see nand.txt for details.  Valid 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
 
 The device tree may optionally contain sub-nodes describing partitions of the
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 8c09bbe..38800ae 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -45,11 +45,42 @@ static const struct denali_dt_data denali_socfpga_data = {
 	},
 };
 
+static const int denali_uniphier_strengths[] = {8, 16, 24};
+static const struct denali_dt_data denali_uniphier_v5a_data = {
+	.caps = DENALI_CAP_HW_ECC_FIXUP |
+		DENALI_CAP_DMA_64BIT,
+	.stepinfo = {
+		.stepsize = 1024,
+		.strengths = denali_uniphier_strengths,
+		.nstrengths = ARRAY_SIZE(denali_uniphier_strengths),
+	},
+};
+
+static const struct denali_dt_data denali_uniphier_v5b_data = {
+	.revision = 0x0501,
+	.caps = DENALI_CAP_HW_ECC_FIXUP |
+		DENALI_CAP_DMA_64BIT,
+	.stepinfo = {
+		.stepsize = 1024,
+		.strengths = denali_uniphier_strengths,
+		/* no support for strength=24 */
+		.nstrengths = ARRAY_SIZE(denali_uniphier_strengths) - 1,
+	},
+};
+
 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 v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Boris Brezillon]

On Tue,  6 Jun 2017 08:21:43 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> This driver was originally written for the Intel MRST platform with
> several platform-specific parameters hard-coded.
> 
> Currently, the ECC settings are hard-coded as follows:
> 
>   #define ECC_SECTOR_SIZE 512
>   #define ECC_8BITS       14
>   #define ECC_15BITS      26
> 
> Therefore, the driver can only support two cases.
>  - ecc.size = 512, ecc.strength = 8    --> ecc.bytes = 14
>  - ecc.size = 512, ecc.strength = 15   --> ecc.bytes = 26
> 
> However, these are actually customizable parameters, for example,
> UniPhier platform supports the following:
> 
>  - ecc.size = 1024, ecc.strength = 8   --> ecc.bytes = 14
>  - ecc.size = 1024, ecc.strength = 16  --> ecc.bytes = 28
>  - ecc.size = 1024, ecc.strength = 24  --> ecc.bytes = 42
> 
> So, we need to handle the ECC parameters in a more generic manner.
> Fortunately, the Denali User's Guide explains how to calculate the
> ecc.bytes.  The formula is:
> 
>   ecc.bytes = 2 * CEIL(13 * ecc.strength / 16)  (for ecc.size = 512)
>   ecc.bytes = 2 * CEIL(14 * ecc.strength / 16)  (for ecc.size = 1024)
> 
> For DT platforms, it would be reasonable to allow DT to specify ECC
> strength by either "nand-ecc-strength" or "nand-ecc-maximize".  If
> none of them is specified, the driver will try to meet the chip's ECC
> requirement.
> 
> For PCI platforms, the max ECC strength is used to keep the original
> behavior.
> 
> 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)
> 
> For older IP versions, write accesses to these registers are just
> ignored.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> 
> Changes in v4:
>   - Rewrite by using generic helpers, nand_check_caps(),
>     nand_match_ecc_req(), nand_maximize_ecc().
> 
> 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        |   7 ++
>  drivers/mtd/nand/denali.c                          | 103 ++++++++++++++-------
>  drivers/mtd/nand/denali.h                          |  11 ++-
>  drivers/mtd/nand/denali_dt.c                       |   8 ++
>  drivers/mtd/nand/denali_pci.c                      |   9 ++
>  5 files changed, 101 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index e593bbe..b7742a7 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -7,6 +7,13 @@ Required properties:
>    - reg-names: Should contain the reg names "nand_data" and "denali_reg"
>    - interrupts : The interrupt number.
>  
> +Optional properties:
> +  - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
> +      512        for "altr,socfpga-denali-nand"
> +  - nand-ecc-strength: see nand.txt for details.  Valid values are:
> +      8, 15      for "altr,socfpga-denali-nand"
> +  - nand-ecc-maximize: 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..3204c51 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 */
> @@ -1345,13 +1344,55 @@ static void denali_hw_init(struct denali_nand_info *denali)
>  	denali_irq_init(denali);
>  }
>  
> -/*
> - * Althogh controller spec said SLC ECC is forceb to be 4bit,
> - * but denali controller in MRST only support 15bit and 8bit ECC
> - * correction
> - */
> -#define ECC_8BITS	14
> -#define ECC_15BITS	26
> +static int denali_calc_ecc_bytes(int step_size, int strength)
> +{
> +	int coef;
> +
> +	switch (step_size) {
> +	case 512:
> +		coef = 13;
> +		break;
> +	case 1024:
> +		coef = 14;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	return DIV_ROUND_UP(strength * coef, 16) * 2;

or just

	return DIV_ROUND_UP(strength * fls(8 * step_size), 16) * 2;

the array of supported step size/strength should guarantee that you're
called with unsupported settings.

> +}
> +
> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> +			    struct denali_nand_info *denali)
> +{
> +	struct nand_ecc_caps caps;
> +	int ret;
> +
> +	caps.stepinfos = denali->stepinfo;
> +	caps.nstepinfos = 1;
> +	caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> +	caps.oob_reserve_bytes = denali->bbtskipbytes;

If you get rid of this oob_reserve_bytes field, you can define caps as
a static const and even directly store ecc_caps in denali_nand_info.

> +
> +	/*
> +	 * If .size and .strength are already set (usually by DT),
> +	 * check if they are supported by this controller.
> +	 */
> +	if (chip->ecc.size && chip->ecc.strength)
> +		return nand_check_ecc_caps(mtd, chip, &caps);
> +
> +	/*
> +	 * We want .size and .strength closest to the chip's requirement
> +	 * unless NAND_ECC_MAXIMIZE is requested.
> +	 */
> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
> +		ret = nand_match_ecc_req(mtd, chip, &caps);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	/* Max ECC strength is the last thing we can do */
> +	return nand_maximize_ecc(mtd, chip, &caps);
> +}
--
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 v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

Hi Masahiro,

On Tue,  6 Jun 2017 08:21:39 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> This patch series intends to solve various 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
> 
> 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.
> 11 patches were merged at v3.
> 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
> v3: https://lkml.org/lkml/2017/3/30/90
> 
> 
> Masahiro Yamada (23):
>   mtd: nand: denali_dt: clean up resource ioremap
>   mtd: nand: denali: use BIT() and GENMASK() for register macros
>   mtd: nand: add generic helpers to check, match, maximize ECC settings
>   mtd: nand: denali: avoid hard-coding ECC step, strength, bytes
>   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: 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 to detect the number of
>     chips
>   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: 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

I'd like to apply as much patches as possible from this series (already
applied patches 1 and 2). Can you point patches that actually depend on
patches 3 and 4?

> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |   13 +
>  drivers/mtd/nand/denali.c                          | 1701 +++++++++-----------
>  drivers/mtd/nand/denali.h                          |  294 ++--
>  drivers/mtd/nand/denali_dt.c                       |   54 +-
>  drivers/mtd/nand/denali_pci.c                      |   15 +-
>  drivers/mtd/nand/nand_base.c                       |  219 +++
>  include/linux/mtd/nand.h                           |   35 +
>  7 files changed, 1235 insertions(+), 1096 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 v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-06-07 7:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Masahiro,
>
> On Tue,  6 Jun 2017 08:21:39 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> This patch series intends to solve various 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
>>
>> 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.
>> 11 patches were merged at v3.
>> 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
>> v3: https://lkml.org/lkml/2017/3/30/90
>>
>>
>> Masahiro Yamada (23):
>>   mtd: nand: denali_dt: clean up resource ioremap
>>   mtd: nand: denali: use BIT() and GENMASK() for register macros
>>   mtd: nand: add generic helpers to check, match, maximize ECC settings
>>   mtd: nand: denali: avoid hard-coding ECC step, strength, bytes
>>   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: 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 to detect the number of
>>     chips
>>   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: 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
>
> I'd like to apply as much patches as possible from this series (already
> applied patches 1 and 2). Can you point patches that actually depend on
> patches 3 and 4?
>


I think
09 "mtd: nand: denali: remove unneeded find_valid_banks()"
is applicable independently.

I will try my best to work on v5 quickly.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Masahiro Yamada]

Hi Boris,


2017-06-07 7:01 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Tue,  6 Jun 2017 08:21:43 +0900
> Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
>> This driver was originally written for the Intel MRST platform with
>> several platform-specific parameters hard-coded.
>>
>> Currently, the ECC settings are hard-coded as follows:
>>
>>   #define ECC_SECTOR_SIZE 512
>>   #define ECC_8BITS       14
>>   #define ECC_15BITS      26
>>
>> Therefore, the driver can only support two cases.
>>  - ecc.size = 512, ecc.strength = 8    --> ecc.bytes = 14
>>  - ecc.size = 512, ecc.strength = 15   --> ecc.bytes = 26
>>
>> However, these are actually customizable parameters, for example,
>> UniPhier platform supports the following:
>>
>>  - ecc.size = 1024, ecc.strength = 8   --> ecc.bytes = 14
>>  - ecc.size = 1024, ecc.strength = 16  --> ecc.bytes = 28
>>  - ecc.size = 1024, ecc.strength = 24  --> ecc.bytes = 42
>>
>> So, we need to handle the ECC parameters in a more generic manner.
>> Fortunately, the Denali User's Guide explains how to calculate the
>> ecc.bytes.  The formula is:
>>
>>   ecc.bytes = 2 * CEIL(13 * ecc.strength / 16)  (for ecc.size = 512)
>>   ecc.bytes = 2 * CEIL(14 * ecc.strength / 16)  (for ecc.size = 1024)
>>
>> For DT platforms, it would be reasonable to allow DT to specify ECC
>> strength by either "nand-ecc-strength" or "nand-ecc-maximize".  If
>> none of them is specified, the driver will try to meet the chip's ECC
>> requirement.
>>
>> For PCI platforms, the max ECC strength is used to keep the original
>> behavior.
>>
>> 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)
>>
>> For older IP versions, write accesses to these registers are just
>> ignored.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>
>> Changes in v4:
>>   - Rewrite by using generic helpers, nand_check_caps(),
>>     nand_match_ecc_req(), nand_maximize_ecc().
>>
>> 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        |   7 ++
>>  drivers/mtd/nand/denali.c                          | 103 ++++++++++++++-------
>>  drivers/mtd/nand/denali.h                          |  11 ++-
>>  drivers/mtd/nand/denali_dt.c                       |   8 ++
>>  drivers/mtd/nand/denali_pci.c                      |   9 ++
>>  5 files changed, 101 insertions(+), 37 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> index e593bbe..b7742a7 100644
>> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> @@ -7,6 +7,13 @@ Required properties:
>>    - reg-names: Should contain the reg names "nand_data" and "denali_reg"
>>    - interrupts : The interrupt number.
>>
>> +Optional properties:
>> +  - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
>> +      512        for "altr,socfpga-denali-nand"
>> +  - nand-ecc-strength: see nand.txt for details.  Valid values are:
>> +      8, 15      for "altr,socfpga-denali-nand"
>> +  - nand-ecc-maximize: 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..3204c51 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 */
>> @@ -1345,13 +1344,55 @@ static void denali_hw_init(struct denali_nand_info *denali)
>>       denali_irq_init(denali);
>>  }
>>
>> -/*
>> - * Althogh controller spec said SLC ECC is forceb to be 4bit,
>> - * but denali controller in MRST only support 15bit and 8bit ECC
>> - * correction
>> - */
>> -#define ECC_8BITS    14
>> -#define ECC_15BITS   26
>> +static int denali_calc_ecc_bytes(int step_size, int strength)
>> +{
>> +     int coef;
>> +
>> +     switch (step_size) {
>> +     case 512:
>> +             coef = 13;
>> +             break;
>> +     case 1024:
>> +             coef = 14;
>> +             break;
>> +     default:
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     return DIV_ROUND_UP(strength * coef, 16) * 2;
>
> or just
>
>         return DIV_ROUND_UP(strength * fls(8 * step_size), 16) * 2;

Good idea.

I heard the Denali ECC engine uses BCH code.
I am not familiar with the algorithm,
but probably this generalized formula is correct.

>> +}
>> +
>> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
>> +                         struct denali_nand_info *denali)
>> +{
>> +     struct nand_ecc_caps caps;
>> +     int ret;
>> +
>> +     caps.stepinfos = denali->stepinfo;
>> +     caps.nstepinfos = 1;
>> +     caps.calc_ecc_bytes = denali_calc_ecc_bytes;
>> +     caps.oob_reserve_bytes = denali->bbtskipbytes;
>
> If you get rid of this oob_reserve_bytes field, you can define caps as
> a static const and even directly store ecc_caps in denali_nand_info.

To make caps static const, denali_calc_ecc_bytes must be exported
to be referenced from denali_dt/denali_pci.
I am reluctant to do it.





-- 
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 v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Boris Brezillon]

On Wed, 7 Jun 2017 12:09:31 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> >> +
> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> >> +                         struct denali_nand_info *denali)
> >> +{
> >> +     struct nand_ecc_caps caps;
> >> +     int ret;
> >> +
> >> +     caps.stepinfos = denali->stepinfo;
> >> +     caps.nstepinfos = 1;
> >> +     caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> >> +     caps.oob_reserve_bytes = denali->bbtskipbytes;  
> >
> > If you get rid of this oob_reserve_bytes field, you can define caps as
> > a static const and even directly store ecc_caps in denali_nand_info.  
> 
> To make caps static const, denali_calc_ecc_bytes must be exported
> to be referenced from denali_dt/denali_pci.
> I am reluctant to do it.

You already duplicate other information in denali_dt.c and
denali_pci.c, so what prevents you from duplicating this one-line
function?

Also, denali core already exports 2 functions, I don't see the problem
in exporting the common nand_ecc_caps object. Why are you reluctant to
that?
--
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 v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Masahiro Yamada]

Hi Boris,


2017-06-07 16:02 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Wed, 7 Jun 2017 12:09:31 +0900
> Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
>> >> +
>> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
>> >> +                         struct denali_nand_info *denali)
>> >> +{
>> >> +     struct nand_ecc_caps caps;
>> >> +     int ret;
>> >> +
>> >> +     caps.stepinfos = denali->stepinfo;
>> >> +     caps.nstepinfos = 1;
>> >> +     caps.calc_ecc_bytes = denali_calc_ecc_bytes;
>> >> +     caps.oob_reserve_bytes = denali->bbtskipbytes;
>> >
>> > If you get rid of this oob_reserve_bytes field, you can define caps as
>> > a static const and even directly store ecc_caps in denali_nand_info.
>>
>> To make caps static const, denali_calc_ecc_bytes must be exported
>> to be referenced from denali_dt/denali_pci.
>> I am reluctant to do it.
>
> You already duplicate other information in denali_dt.c and
> denali_pci.c,

The ECC step-size and strength are tightly associated to each IP variant.
I see duplication between denali_dt and denali_pci, but it is just because
Intel and Altera happened to have the same parameters.

On the other hand, denali_calc_ecc_bytes() is common to all variants
because ECC algorithm is not customizable.


> so what prevents you from duplicating this one-line
> function?
>
> Also, denali core already exports 2 functions,

They are entries for probe/remove.

> I don't see the problem
> in exporting the common nand_ecc_caps object. Why are you reluctant to
> that?

denali_calc_ecc_bytes() is independent of DT, PCI, or whatever.
I see less reason to expose it.

caps is only used on probing, so I used a local variable.
I do not think it is a big problem.

-- 
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 v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

On Wed, 7 Jun 2017 10:21:07 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> 
> 2017-06-07 7:09 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > Hi Masahiro,
> >
> > On Tue,  6 Jun 2017 08:21:39 +0900
> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >  
> >> This patch series intends to solve various 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
> >>
> >> 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.
> >> 11 patches were merged at v3.
> >> 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
> >> v3: https://lkml.org/lkml/2017/3/30/90
> >>
> >>
> >> Masahiro Yamada (23):
> >>   mtd: nand: denali_dt: clean up resource ioremap
> >>   mtd: nand: denali: use BIT() and GENMASK() for register macros
> >>   mtd: nand: add generic helpers to check, match, maximize ECC settings
> >>   mtd: nand: denali: avoid hard-coding ECC step, strength, bytes
> >>   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: 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 to detect the number of
> >>     chips
> >>   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: 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  
> >
> > I'd like to apply as much patches as possible from this series (already
> > applied patches 1 and 2). Can you point patches that actually depend on
> > patches 3 and 4?
> >  
> 
> 
> I think
> 09 "mtd: nand: denali: remove unneeded find_valid_banks()"
> is applicable independently.
> 
> I will try my best to work on v5 quickly.

Applied patches 1 and 2. Can you send only patches 3 and 4 in your v5
or re-order patches to avoid the dependency on these patches.


--
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 v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes

[Boris Brezillon]

On Wed, 7 Jun 2017 16:21:15 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> 
> 2017-06-07 16:02 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Wed, 7 Jun 2017 12:09:31 +0900
> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >  
> >> >> +
> >> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> >> >> +                         struct denali_nand_info *denali)
> >> >> +{
> >> >> +     struct nand_ecc_caps caps;
> >> >> +     int ret;
> >> >> +
> >> >> +     caps.stepinfos = denali->stepinfo;
> >> >> +     caps.nstepinfos = 1;
> >> >> +     caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> >> >> +     caps.oob_reserve_bytes = denali->bbtskipbytes;  
> >> >
> >> > If you get rid of this oob_reserve_bytes field, you can define caps as
> >> > a static const and even directly store ecc_caps in denali_nand_info.  
> >>
> >> To make caps static const, denali_calc_ecc_bytes must be exported
> >> to be referenced from denali_dt/denali_pci.
> >> I am reluctant to do it.  
> >
> > You already duplicate other information in denali_dt.c and
> > denali_pci.c,  
> 
> The ECC step-size and strength are tightly associated to each IP variant.
> I see duplication between denali_dt and denali_pci, but it is just because
> Intel and Altera happened to have the same parameters.

It's still duplication.

> 
> On the other hand, denali_calc_ecc_bytes() is common to all variants
> because ECC algorithm is not customizable.

Yes, I agree.

> 
> 
> > so what prevents you from duplicating this one-line
> > function?
> >
> > Also, denali core already exports 2 functions,  
> 
> They are entries for probe/remove.
> 
> > I don't see the problem
> > in exporting the common nand_ecc_caps object. Why are you reluctant to
> > that?  
> 
> denali_calc_ecc_bytes() is independent of DT, PCI, or whatever.
> I see less reason to expose it.

I don't get that one. The fact that it's a generic implementation makes
it a good match for something you want to have in the core and expose
to DT/PCI implems.

> 
> caps is only used on probing, so I used a local variable.
> I do not think it is a big problem.
> 

It is to me, because you'll be the only user of the API at first, and
people tend to copy&paste code from other drivers.
nand_ecc_caps is really something that should be const and attached to
a specific IP revision.
--
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