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

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

[Masahiro Yamada]

As I said in the 1st round series, I am tackling on this driver
to use it for my SoCs.

The previous series was just cosmetic things, but this series
includes *real* changes.

After some more cleanups, I will start to add changes that
are really necessary.
One of the biggest problems I want to solve is a bunch of
hard-coded parameters that prevent me from using this driver for
my SoCs.

I will introduce capability flags that are associated with DT
compatible and make platform-dependent parameters overridable.

I still have lots of reworks to get done (so probably 3rd round
series will come), but I hope it is getting better and
I am showing a big picture now.



Masahiro Yamada (39):
  mtd: nand: allow to set only one of ECC size and ECC strength from DT
  mtd: nand: denali: remove unused CONFIG option and macros
  mtd: nand: denali: remove redundant define of BANK(x)
  mtd: nand: denali: remove more unused struct members
  mtd: nand: denali: fix comment of denali_nand_info::flash_mem
  mtd: nand: denali: fix write_oob_data() function
  mtd: nand: denali: transfer OOB only when oob_required is set
  mtd: nand: denali: introduce capability flag
  mtd: nand: denali: fix erased page check code
  mtd: nand: denali: remove redundant if conditional of erased_check
  mtd: nand: denali: increment ecc_stats.failed by one per error
  mtd: nand: denali: return 0 for uncorrectable ECC error
  mtd: nand: denali: increment ecc_stats->corrected
  mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
  mtd: nand: denali: improve readability of handle_ecc()
  mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
  mtd: nand: denali: support HW_ECC_FIXUP capability
  mtd: nand: denali: move denali_read_page_raw() above
    denali_read_page()
  mtd: nand: denali: perform erased check against raw transferred page
  mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
  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: add NEW_N_BANKS_FORMAT capability
  mtd: nand: denali: use nand_chip to hold frequently accessed data
  mtd: nand: denali: call nand_set_flash_node() to set DT node
  mtd: nand: denali: do not set mtd->name
  mtd: nand: denali: move multi NAND fixup code to a helper function
  mtd: nand: denali: refactor multi NAND fixup code in more generic way
  mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
  mtd: nand: denali: remove meaningless writes to read-only registers
  mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
  mtd: nand: denali: support 1024 byte ECC step size
  mtd: nand: denali: fix the condition for 15 bit ECC strength
  mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
  mtd: nand: denali: allow to use SoC-specific ECC strength
  mtd: nand: denali: support "nand-ecc-strength" DT property
  mtd: nand: denali: remove Toshiba, Hynix specific fixup code
  mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

 .../devicetree/bindings/mtd/denali-nand.txt        |  19 +-
 drivers/mtd/nand/Kconfig                           |  11 -
 drivers/mtd/nand/denali.c                          | 740 ++++++++++++---------
 drivers/mtd/nand/denali.h                          |  84 +--
 drivers/mtd/nand/denali_dt.c                       |  95 ++-
 drivers/mtd/nand/denali_pci.c                      |   2 +
 drivers/mtd/nand/nand_base.c                       |   6 -
 7 files changed, 515 insertions(+), 442 deletions(-)

-- 
2.7.4

[PATCH 22/39] mtd: nand: denali_dt: remove dma-mask DT property

[Masahiro Yamada]

The driver sets appropriate DMA mask.  Delete the "dma-mask" DT
property.  Refer to the Link tag for negative opinions for this
binding.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Link: https://lkml.org/lkml/2016/2/8/57
---

 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 b04d03a..603110b 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -5,7 +5,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.
@@ -19,5 +18,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 9dcd203..6a486a7 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -40,8 +40,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;
@@ -79,13 +77,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 33/39] 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 1024 byte ECC
size as well.  Add a new flag DENALI_CAPS_ECC_SIZE_1024. If it is
specified, ecc.size is set to 1024, otherwise set to 512.

We can use "nand-ecc-step-size" DT property to override the ecc.size
if we want, but this capability flag can provide the reasonable
default because it is associated with the DT compatible strings.

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

 .../devicetree/bindings/mtd/denali-nand.txt        |  4 ++++
 drivers/mtd/nand/denali.c                          | 26 +++++++++++-----------
 drivers/mtd/nand/denali.h                          |  3 +--
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 603110b..e9d5818 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -6,6 +6,10 @@ 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.
+    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 63f7500..5d80f16 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -894,8 +894,6 @@ static bool denali_hw_ecc_fixup(struct denali_nand_info *denali,
 	return false;
 }
 
-#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)
@@ -908,6 +906,7 @@ static bool denali_sw_ecc_fixup(struct denali_nand_info *denali, u8 *buf,
 {
 	bool check_erased_page = false;
 	unsigned int bitflips = 0;
+	unsigned int ecc_size = denali->nand.ecc.size;
 	u32 err_address, err_correction_info, err_byte, err_sector, err_device,
 	    err_correction_value;
 
@@ -930,18 +929,18 @@ static bool denali_sw_ecc_fixup(struct denali_nand_info *denali, u8 *buf,
 
 		if (ECC_ERROR_CORRECTABLE(err_correction_info)) {
 			/*
-			 * 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
 			 * one NAND connected.
 			 */
-			if (err_byte < ECC_SECTOR_SIZE) {
+			if (err_byte < ecc_size) {
 				struct mtd_info *mtd =
 					nand_to_mtd(&denali->nand);
 				int offset;
 
-				offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
+				offset = (err_sector * ecc_size + err_byte) *
 					denali->devnum + err_device;
 				/* correct the ECC error */
 				buf[offset] ^= err_correction_value;
@@ -1590,22 +1589,25 @@ int denali_init(struct denali_nand_info *denali)
 	/* no subpage writes on denali */
 	chip->options |= NAND_NO_SUBPAGE_WRITE;
 
+	/* If "nand-ecc-step-size" DT property is specified, respect it */
+	if (!chip->ecc.size)
+		chip->ecc.size = denali->caps & DENALI_CAPS_ECC_SIZE_1024 ?
+								1024 : 512;
+
 	/*
 	 * 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 {
@@ -1616,8 +1618,6 @@ int denali_init(struct denali_nand_info *denali)
 
 	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 d621b74..5209625 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -396,8 +396,6 @@
 #define MODE_10    0x08000000
 #define MODE_11    0x0C000000
 
-#define ECC_SECTOR_SIZE     512
-
 struct nand_buf {
 	int head;
 	int tail;
@@ -434,6 +432,7 @@ struct denali_nand_info {
 #define DENALI_CAPS_HW_ECC_FIXUP		BIT(0)
 #define DENALI_CAPS_DMA_64BIT			BIT(1)
 #define DENALI_CAPS_NEW_N_BANKS_FORMAT		BIT(2)
+#define DENALI_CAPS_ECC_SIZE_1024		BIT(3)
 };
 
 extern int denali_init(struct denali_nand_info *denali);
-- 
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 37/39] 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.

Going forward, DT platforms should specify "nand-ecc-strength" or
"nand-ecc-maximize" to show the ECC strength strategy explicitly.

If nothing is specified in DT, "nand-ecc-maximize" is implied since
this was the original behavior.  It applies to PCI platforms too.

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

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 ++++
 drivers/mtd/nand/denali.c                          | 27 +++++++++++++++++++++-
 drivers/mtd/nand/denali_pci.c                      |  2 ++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index e9d5818..51fe195 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -9,6 +9,11 @@ Required properties:
 Optional properties:
   - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to 512.
     see nand.txt for details.
+  - nand-ecc-strength: see nand.txt for details
+  - 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 54c9e0c..df174ca 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1393,6 +1393,21 @@ static int denali_set_max_ecc_strength(struct denali_nand_info *denali)
 	return -EINVAL;
 }
 
+static int denali_check_ecc_strength(struct denali_nand_info *denali)
+{
+	const int *ecc_strength = denali->ecc_strength_avail;
+
+	for (; *ecc_strength; ecc_strength++) {
+		if (*ecc_strength == denali->nand.ecc.strength)
+			return 0;
+	}
+
+	dev_err(denali->dev,
+		"Specified ECC strength is not supported for this controller.\n");
+
+	return -EINVAL;
+}
+
 static int denali_ooblayout_ecc(struct mtd_info *mtd, int section,
 				struct mtd_oob_region *oobregion)
 {
@@ -1628,7 +1643,17 @@ int denali_init(struct denali_nand_info *denali)
 	if (!denali->ecc_strength_avail)
 		denali->ecc_strength_avail = denali_default_ecc_strength;
 
-	ret = denali_set_max_ecc_strength(denali);
+	if (!chip->ecc.strength && !(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
+		dev_info(denali->dev,
+			"No ECC strength is specified.  Trying max ECC strength strategy\n");
+		chip->ecc.options |= NAND_ECC_MAXIMIZE;
+	}
+
+	if (chip->ecc.options & NAND_ECC_MAXIMIZE)
+		ret = denali_set_max_ecc_strength(denali);
+	else
+		ret = denali_check_ecc_strength(denali);
+
 	if (ret)
 		goto failed_req_irq;
 
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index ac84323..0064f3e 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->nand.ecc.options |= NAND_ECC_MAXIMIZE;
+
 	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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Add two compatible strings for UniPhier SoCs.  The revision register
on both shows revision 5.0, but they are different hardware.

Features:
 - DMA engine with 64 bit physical address support
 - 1024 byte ECC step size
 - 8 / 16 / 24 bit ECC strength
 - The n_banks format depends on SoC

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

 .../devicetree/bindings/mtd/denali-nand.txt        | 10 +++++--
 drivers/mtd/nand/denali_dt.c                       | 33 ++++++++++++++++++++--
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 51fe195..cea46e2 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -1,13 +1,19 @@
 * Denali NAND controller
 
 Required properties:
-  - compatible : should be "denali,denali-nand-dt"
+  - compatible : should be one of the following:
+      "denali,denali-nand-dt"
+      "denali,denali-nand-uniphier-v5a"
+      "denali,denali-nand-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.
 
 Optional properties:
-  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to 512.
+  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to:
+      512  for "denali,denali-nand-dt"
+      1024 for "denali,denali-nand-uniphier-v5a"
+      1024 for "denali,denali-nand-uniphier-v5b"
     see nand.txt for details.
   - nand-ecc-strength: see nand.txt for details
   - nand-ecc-maximize: see nand.txt for details
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index aa1e032..b411889 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -34,10 +34,37 @@ struct denali_dt_data {
 	unsigned int caps;
 };
 
+static const int denali_uniphier_ecc_strength[] = {
+	24, 16, 8, 0,
+};
+
+static const struct denali_dt_data denali_uniphier_v5a_data = {
+	.ecc_strength_avail = denali_uniphier_ecc_strength,
+	.caps = DENALI_CAPS_DMA_64BIT |
+		DENALI_CAPS_ECC_SIZE_1024,
+};
+
+static const struct denali_dt_data denali_uniphier_v5b_data = {
+	.ecc_strength_avail = denali_uniphier_ecc_strength,
+	.caps = DENALI_CAPS_DMA_64BIT |
+		DENALI_CAPS_NEW_N_BANKS_FORMAT |
+		DENALI_CAPS_ECC_SIZE_1024,
+};
+
 static const struct of_device_id denali_nand_dt_ids[] = {
-		{ .compatible = "denali,denali-nand-dt" },
-		{ /* sentinel */ }
-	};
+	{
+		.compatible = "denali,denali-nand-dt",
+	},
+	{
+		.compatible = "denali,denali-nand-uniphier-v5a",
+		.data = &denali_uniphier_v5a_data,
+	},
+	{
+		.compatible = "denali,denali-nand-uniphier-v5b",
+		.data = &denali_uniphier_v5b_data,
+	},
+	{ /* sentinel */ }
+};
 
 MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
 
-- 
2.7.4

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

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

[Boris Brezillon]

+Andy

Hi Masahiro,

On Sun, 27 Nov 2016 03:05:46 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> As I said in the 1st round series, I am tackling on this driver
> to use it for my SoCs.
> 
> The previous series was just cosmetic things, but this series
> includes *real* changes.
> 
> After some more cleanups, I will start to add changes that
> are really necessary.
> One of the biggest problems I want to solve is a bunch of
> hard-coded parameters that prevent me from using this driver for
> my SoCs.
> 
> I will introduce capability flags that are associated with DT
> compatible and make platform-dependent parameters overridable.
> 
> I still have lots of reworks to get done (so probably 3rd round
> series will come), but I hope it is getting better and
> I am showing a big picture now.
> 

Thanks for posting this 2nd round of patches, I know have a clearer
view of what you're trying to achieve.
Could you be a bit more specific about the remaining rework (your 3rd
round)?

Also, if you don't mind, I'd like to have reviews and testing from intel
users before applying the series. Can you Cc Andy (and possibly other
intel maintainers) for the next round.

Thanks,

Boris

> 
> 
> Masahiro Yamada (39):
>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
>   mtd: nand: denali: remove unused CONFIG option and macros
>   mtd: nand: denali: remove redundant define of BANK(x)
>   mtd: nand: denali: remove more unused struct members
>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
>   mtd: nand: denali: fix write_oob_data() function
>   mtd: nand: denali: transfer OOB only when oob_required is set
>   mtd: nand: denali: introduce capability flag
>   mtd: nand: denali: fix erased page check code
>   mtd: nand: denali: remove redundant if conditional of erased_check
>   mtd: nand: denali: increment ecc_stats.failed by one per error
>   mtd: nand: denali: return 0 for uncorrectable ECC error
>   mtd: nand: denali: increment ecc_stats->corrected
>   mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
>   mtd: nand: denali: improve readability of handle_ecc()
>   mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
>   mtd: nand: denali: support HW_ECC_FIXUP capability
>   mtd: nand: denali: move denali_read_page_raw() above
>     denali_read_page()
>   mtd: nand: denali: perform erased check against raw transferred page
>   mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
>   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: add NEW_N_BANKS_FORMAT capability
>   mtd: nand: denali: use nand_chip to hold frequently accessed data
>   mtd: nand: denali: call nand_set_flash_node() to set DT node
>   mtd: nand: denali: do not set mtd->name
>   mtd: nand: denali: move multi NAND fixup code to a helper function
>   mtd: nand: denali: refactor multi NAND fixup code in more generic way
>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
>   mtd: nand: denali: remove meaningless writes to read-only registers
>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
>   mtd: nand: denali: support 1024 byte ECC step size
>   mtd: nand: denali: fix the condition for 15 bit ECC strength
>   mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
>   mtd: nand: denali: allow to use SoC-specific ECC strength
>   mtd: nand: denali: support "nand-ecc-strength" DT property
>   mtd: nand: denali: remove Toshiba, Hynix specific fixup code
>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  19 +-
>  drivers/mtd/nand/Kconfig                           |  11 -
>  drivers/mtd/nand/denali.c                          | 740 ++++++++++++---------
>  drivers/mtd/nand/denali.h                          |  84 +--
>  drivers/mtd/nand/denali_dt.c                       |  95 ++-
>  drivers/mtd/nand/denali_pci.c                      |   2 +
>  drivers/mtd/nand/nand_base.c                       |   6 -
>  7 files changed, 515 insertions(+), 442 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 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

On Sun, 27 Nov 2016 03:05:46 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> As I said in the 1st round series, I am tackling on this driver
> to use it for my SoCs.
> 
> The previous series was just cosmetic things, but this series
> includes *real* changes.
> 
> After some more cleanups, I will start to add changes that
> are really necessary.
> One of the biggest problems I want to solve is a bunch of
> hard-coded parameters that prevent me from using this driver for
> my SoCs.
> 
> I will introduce capability flags that are associated with DT
> compatible and make platform-dependent parameters overridable.
> 
> I still have lots of reworks to get done (so probably 3rd round
> series will come), but I hope it is getting better and
> I am showing a big picture now.
> 

I still need to carefully review some of those patches, but I must
admit I like some of the cleanups/rework you're doing here.

Thanks for all your work.

Boris

> 
> 
> Masahiro Yamada (39):
>   mtd: nand: allow to set only one of ECC size and ECC strength from DT
>   mtd: nand: denali: remove unused CONFIG option and macros
>   mtd: nand: denali: remove redundant define of BANK(x)
>   mtd: nand: denali: remove more unused struct members
>   mtd: nand: denali: fix comment of denali_nand_info::flash_mem
>   mtd: nand: denali: fix write_oob_data() function
>   mtd: nand: denali: transfer OOB only when oob_required is set
>   mtd: nand: denali: introduce capability flag
>   mtd: nand: denali: fix erased page check code
>   mtd: nand: denali: remove redundant if conditional of erased_check
>   mtd: nand: denali: increment ecc_stats.failed by one per error
>   mtd: nand: denali: return 0 for uncorrectable ECC error
>   mtd: nand: denali: increment ecc_stats->corrected
>   mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
>   mtd: nand: denali: improve readability of handle_ecc()
>   mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
>   mtd: nand: denali: support HW_ECC_FIXUP capability
>   mtd: nand: denali: move denali_read_page_raw() above
>     denali_read_page()
>   mtd: nand: denali: perform erased check against raw transferred page
>   mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
>   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: add NEW_N_BANKS_FORMAT capability
>   mtd: nand: denali: use nand_chip to hold frequently accessed data
>   mtd: nand: denali: call nand_set_flash_node() to set DT node
>   mtd: nand: denali: do not set mtd->name
>   mtd: nand: denali: move multi NAND fixup code to a helper function
>   mtd: nand: denali: refactor multi NAND fixup code in more generic way
>   mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
>   mtd: nand: denali: remove meaningless writes to read-only registers
>   mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
>   mtd: nand: denali: support 1024 byte ECC step size
>   mtd: nand: denali: fix the condition for 15 bit ECC strength
>   mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
>   mtd: nand: denali: allow to use SoC-specific ECC strength
>   mtd: nand: denali: support "nand-ecc-strength" DT property
>   mtd: nand: denali: remove Toshiba, Hynix specific fixup code
>   mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  19 +-
>  drivers/mtd/nand/Kconfig                           |  11 -
>  drivers/mtd/nand/denali.c                          | 740 ++++++++++++---------
>  drivers/mtd/nand/denali.h                          |  84 +--
>  drivers/mtd/nand/denali_dt.c                       |  95 ++-
>  drivers/mtd/nand/denali_pci.c                      |   2 +
>  drivers/mtd/nand/nand_base.c                       |   6 -
>  7 files changed, 515 insertions(+), 442 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 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Masahiro Yamada]

Hi.

2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> +Andy
>
> Hi Masahiro,
>
> On Sun, 27 Nov 2016 03:05:46 +0900
> Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
>
>> As I said in the 1st round series, I am tackling on this driver
>> to use it for my SoCs.
>>
>> The previous series was just cosmetic things, but this series
>> includes *real* changes.
>>
>> After some more cleanups, I will start to add changes that
>> are really necessary.
>> One of the biggest problems I want to solve is a bunch of
>> hard-coded parameters that prevent me from using this driver for
>> my SoCs.
>>
>> I will introduce capability flags that are associated with DT
>> compatible and make platform-dependent parameters overridable.
>>
>> I still have lots of reworks to get done (so probably 3rd round
>> series will come), but I hope it is getting better and
>> I am showing a big picture now.
>>
>
> Thanks for posting this 2nd round of patches, I know have a clearer
> view of what you're trying to achieve.
> Could you be a bit more specific about the remaining rework (your 3rd
> round)?


[1]
I want to remove
get_samsung_nand_para()
get_onfi_nand_para()

The driver should not hard-code timing parameters of Samsung specific
chips.  For ONFI, it is duplicating effort of the core framework.

I am thinking if it would be possible to implement
chip->setup_data_interface() in order to set up
timings in a generic way.

[2]
Remove driver-internal bounce buffer.
The current Denali driver allocate DMA_BIDIRECTIONAL buffer
to use it as a driver-internal bounce buffer.

The hardware transfer page data into the bounce buffer,
then CPU copies from the bounce buffer to a given buf (and oob_poi).
This is not efficient.

So, I want to set NAND_USE_BOUNCE_BUFFER flag
and do dma_map_single directly for a given buffer.

[3]
Fix raw and oob callbacks.

I asked in another thread,
the current driver just puts the physically accessed OOB data
into oob_poi, which is not a collection of ECC data.
Raw write/read() are wrong as well.

After fixing those, enable BBT scan by removing the following flag:
    /* skip the scan for now until we have OOB read and write support */
    chip->options |= NAND_SKIP_BBTSCAN;



> Also, if you don't mind, I'd like to have reviews and testing from intel
> users before applying the series. Can you Cc Andy (and possibly other
> intel maintainers) for the next round.

Sure.

Anyway, this series already missed the pull-req for 4.10-rc1,
we have plenty of time until 4.11-rc1.

Review/test from Intel engineers are very appreciated
because I have no access to their boards.


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

[Masahiro Yamada]

2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> +Andy
>
> Hi Masahiro,
>
> On Sun, 27 Nov 2016 03:05:46 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> As I said in the 1st round series, I am tackling on this driver
>> to use it for my SoCs.
>>
>> The previous series was just cosmetic things, but this series
>> includes *real* changes.
>>
>> After some more cleanups, I will start to add changes that
>> are really necessary.
>> One of the biggest problems I want to solve is a bunch of
>> hard-coded parameters that prevent me from using this driver for
>> my SoCs.
>>
>> I will introduce capability flags that are associated with DT
>> compatible and make platform-dependent parameters overridable.
>>
>> I still have lots of reworks to get done (so probably 3rd round
>> series will come), but I hope it is getting better and
>> I am showing a big picture now.
>>
>
> Thanks for posting this 2nd round of patches, I know have a clearer
> view of what you're trying to achieve.
> Could you be a bit more specific about the remaining rework (your 3rd
> round)?


We still have plenty of time, and no reason to hurry now.
So, you do not have to apply this series
until you see an even bigger picture.

I will try my best to include as many of my plans as possible
in this round.

I may end up with dropping my on-going work to the ML occasionally
because I want early feedback in case I am doing something wrong.

-- 
Best Regards
Masahiro Yamada

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

[Boris Brezillon]

On Wed, 30 Nov 2016 17:02:16 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi.
> 
> 2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > +Andy
> >
> > Hi Masahiro,
> >
> > On Sun, 27 Nov 2016 03:05:46 +0900
> > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> >  
> >> As I said in the 1st round series, I am tackling on this driver
> >> to use it for my SoCs.
> >>
> >> The previous series was just cosmetic things, but this series
> >> includes *real* changes.
> >>
> >> After some more cleanups, I will start to add changes that
> >> are really necessary.
> >> One of the biggest problems I want to solve is a bunch of
> >> hard-coded parameters that prevent me from using this driver for
> >> my SoCs.
> >>
> >> I will introduce capability flags that are associated with DT
> >> compatible and make platform-dependent parameters overridable.
> >>
> >> I still have lots of reworks to get done (so probably 3rd round
> >> series will come), but I hope it is getting better and
> >> I am showing a big picture now.
> >>  
> >
> > Thanks for posting this 2nd round of patches, I know have a clearer
> > view of what you're trying to achieve.
> > Could you be a bit more specific about the remaining rework (your 3rd
> > round)?  
> 
> 
> [1]
> I want to remove
> get_samsung_nand_para()
> get_onfi_nand_para()
> 
> The driver should not hard-code timing parameters of Samsung specific
> chips.  For ONFI, it is duplicating effort of the core framework.

Definitely.

> 
> I am thinking if it would be possible to implement
> chip->setup_data_interface() in order to set up
> timings in a generic way.

Indeed, and that'd be really cool to have this driver converted to this
new interface.

> 
> [2]
> Remove driver-internal bounce buffer.
> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
> to use it as a driver-internal bounce buffer.
> 
> The hardware transfer page data into the bounce buffer,
> then CPU copies from the bounce buffer to a given buf (and oob_poi).
> This is not efficient.
> 
> So, I want to set NAND_USE_BOUNCE_BUFFER flag
> and do dma_map_single directly for a given buffer.

Sounds good. Be careful though, when you use the generic bounce buffer
interface you might have to clear the page cache info (->pagebuf = -1).

> 
> [3]
> Fix raw and oob callbacks.
> 
> I asked in another thread,
> the current driver just puts the physically accessed OOB data
> into oob_poi, which is not a collection of ECC data.
> Raw write/read() are wrong as well.

That's all good things too.

> 
> After fixing those, enable BBT scan by removing the following flag:
>     /* skip the scan for now until we have OOB read and write support */
>     chip->options |= NAND_SKIP_BBTSCAN;
> 

Hm, here you have a problem. The layout you described replaces BBMs by
payload data, thus preventing the BBM scan approach (or at least, it
won't work with factory BBMs).

Some drivers/controllers have an extra 'switch BBM/data bytes' step to
restore the BBM at the correct place before flushing the data to the
NAND or after reading a page, but I'm not sure this is the case here.

> 
> 
> > Also, if you don't mind, I'd like to have reviews and testing from intel
> > users before applying the series. Can you Cc Andy (and possibly other
> > intel maintainers) for the next round.  
> 
> Sure.
> 
> Anyway, this series already missed the pull-req for 4.10-rc1,
> we have plenty of time until 4.11-rc1.
> 
> Review/test from Intel engineers are very appreciated
> because I have no access to their boards.
> 
> 

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

[Masahiro Yamada]

Hi Boris,

2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:

>> [3]
>> Fix raw and oob callbacks.
>>
>> I asked in another thread,
>> the current driver just puts the physically accessed OOB data
>> into oob_poi, which is not a collection of ECC data.
>> Raw write/read() are wrong as well.
>
> That's all good things too.
>
>>
>> After fixing those, enable BBT scan by removing the following flag:
>>     /* skip the scan for now until we have OOB read and write support */
>>     chip->options |= NAND_SKIP_BBTSCAN;
>>
>
> Hm, here you have a problem. The layout you described replaces BBMs by
> payload data, thus preventing the BBM scan approach (or at least, it
> won't work with factory BBMs).


As I answered in another mail, the Denali IP expects BBMs
at the beginning of each OOB area (standard location).
They are protected from the ECC engine.

I just did not mention the BBM-reserved area
to make the story simpler.

So, after fixing oob read/write functions,
the driver will be able to enable BBT-scanning.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 22/39] mtd: nand: denali_dt: remove dma-mask DT property

[Rob Herring]

On Sun, Nov 27, 2016 at 03:06:08AM +0900, Masahiro Yamada wrote:
> The driver sets appropriate DMA mask.  Delete the "dma-mask" DT
> property.  Refer to the Link tag for negative opinions for this
> binding.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Link: https://lkml.org/lkml/2016/2/8/57
> ---
> 
>  Documentation/devicetree/bindings/mtd/denali-nand.txt | 2 --
>  drivers/mtd/nand/denali_dt.c                          | 9 ---------
>  2 files changed, 11 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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 33/39] mtd: nand: denali: support 1024 byte ECC step size

[Rob Herring]

On Sun, Nov 27, 2016 at 03:06:19AM +0900, Masahiro Yamada wrote:
> This driver was originally written for the Intel MRST platform with
> several platform specific parameters hard-coded.  Another thing we
> need to fix is the hard-coded ECC step size.  Currently, it is
> defined as follows:
> 
>   #define ECC_SECTOR_SIZE 512
> 
> (somehow, it is defined in both denali.c and denali.h)
> 
> This must be avoided because the Denali IP supports 1024 byte ECC
> size as well.  Add a new flag DENALI_CAPS_ECC_SIZE_1024. If it is
> specified, ecc.size is set to 1024, otherwise set to 512.
> 
> We can use "nand-ecc-step-size" DT property to override the ecc.size
> if we want, but this capability flag can provide the reasonable
> default because it is associated with the DT compatible strings.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> ---
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  4 ++++

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

>  drivers/mtd/nand/denali.c                          | 26 +++++++++++-----------
>  drivers/mtd/nand/denali.h                          |  3 +--
>  3 files changed, 18 insertions(+), 15 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 37/39] mtd: nand: denali: support "nand-ecc-strength" DT property

[Rob Herring]

On Sun, Nov 27, 2016 at 03:06:23AM +0900, Masahiro Yamada 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.
> 
> Going forward, DT platforms should specify "nand-ecc-strength" or
> "nand-ecc-maximize" to show the ECC strength strategy explicitly.
> 
> If nothing is specified in DT, "nand-ecc-maximize" is implied since
> this was the original behavior.  It applies to PCI platforms too.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  5 ++++

I'd prefer all the DT changes be in 1 patch, but

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

>  drivers/mtd/nand/denali.c                          | 27 +++++++++++++++++++++-
>  drivers/mtd/nand/denali_pci.c                      |  2 ++
>  3 files changed, 33 insertions(+), 1 deletion(-)

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

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Rob Herring]

On Sun, Nov 27, 2016 at 03:06:25AM +0900, Masahiro Yamada wrote:
> Add two compatible strings for UniPhier SoCs.  The revision register
> on both shows revision 5.0, but they are different hardware.
> 
> Features:
>  - DMA engine with 64 bit physical address support
>  - 1024 byte ECC step size
>  - 8 / 16 / 24 bit ECC strength
>  - The n_banks format depends on SoC
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        | 10 +++++--
>  drivers/mtd/nand/denali_dt.c                       | 33 ++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 51fe195..cea46e2 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -1,13 +1,19 @@
>  * Denali NAND controller
>  
>  Required properties:
> -  - compatible : should be "denali,denali-nand-dt"
> +  - compatible : should be one of the following:
> +      "denali,denali-nand-dt"

There are multiple things wrong with this string. denali,denali is 
redundant is one. It's also fairly useless as this IP has several 
versions and numerous configuration options IIRC. This should be 
deprecated IMO.

> +      "denali,denali-nand-uniphier-v5a"
> +      "denali,denali-nand-uniphier-v5b"

Use your vendor prefix, not denali. The 2nd denali can probably be 
dropped because it is not likely you have another kind of nand 
controller in the SoC.

>    - 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.
>  
>  Optional properties:
> -  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to 512.
> +  - nand-ecc-step-size: must be 512 or 1024.  If not specified, default to:
> +      512  for "denali,denali-nand-dt"
> +      1024 for "denali,denali-nand-uniphier-v5a"
> +      1024 for "denali,denali-nand-uniphier-v5b"
>      see nand.txt for details.
>    - nand-ecc-strength: see nand.txt for details
>    - nand-ecc-maximize: see nand.txt for details

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Hi Rob,
(+CC Dinh)

2016-12-02 1:05 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Sun, Nov 27, 2016 at 03:06:25AM +0900, Masahiro Yamada wrote:
>> Add two compatible strings for UniPhier SoCs.  The revision register
>> on both shows revision 5.0, but they are different hardware.
>>
>> Features:
>>  - DMA engine with 64 bit physical address support
>>  - 1024 byte ECC step size
>>  - 8 / 16 / 24 bit ECC strength
>>  - The n_banks format depends on SoC
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
>> ---
>>
>>  .../devicetree/bindings/mtd/denali-nand.txt        | 10 +++++--
>>  drivers/mtd/nand/denali_dt.c                       | 33 ++++++++++++++++++++--
>>  2 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> index 51fe195..cea46e2 100644
>> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> @@ -1,13 +1,19 @@
>>  * Denali NAND controller
>>
>>  Required properties:
>> -  - compatible : should be "denali,denali-nand-dt"
>> +  - compatible : should be one of the following:
>> +      "denali,denali-nand-dt"
>
> There are multiple things wrong with this string. denali,denali is
> redundant is one.

One more redundancy; "-dt" is weird because
DT compatible should be a name of hardware.


> It's also fairly useless as this IP has several
> versions and numerous configuration options IIRC. This should be
> deprecated IMO.

Right.  There are several customizable parameters for this IP,
so a generic compatible string like this is probably useless.

This DT binding was added by commit 30f9f2f for Altera SOCFPGA,
A funny thing is that they upstreamed DT binding, but they did not upstream
needed changes for the Denali driver core.
So, the mainline driver has never worked on SOCFPGA
(or any of DT-based SoCs).



>> +      "denali,denali-nand-uniphier-v5a"
>> +      "denali,denali-nand-uniphier-v5b"
>
> Use your vendor prefix, not denali. The 2nd denali can probably be
> dropped because it is not likely you have another kind of nand
> controller in the SoC.

Hmm, your statement implies that a vendor prefix
belongs to an SoC vendor, not an IP vendor.
(I was not quite sure about this.)


It is unlikely to happen to have two different NAND controllers on one SoC.
But, we used a different NAND controller for our SoC family before
introducing the Denali IP.
It also implies that Socionext may use a different NAND IP in the future.
I'd like to include "denali" somewhere because it is clearly associated with
the driver name.
Also, this will give an idea what kind of _basic_ hardware is used,
even though we know various parameters are customizable.



(Plan A)
  "denali,socfpga-nand"           (for Altera SOCFPGA variant)
  "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
  "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)

(Plan B)
  "altera,denali-nand"            (for Altera SOCFPGA variant)
  "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
  "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)


I think Plan B is nearer to your suggestion,
and I think it is OK for Socionext (hopefully for Altera too).





-- 
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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Rob Herring]

On Thu, Dec 1, 2016 at 8:54 PM, Masahiro Yamada
<yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> Hi Rob,
> (+CC Dinh)
>
> 2016-12-02 1:05 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> On Sun, Nov 27, 2016 at 03:06:25AM +0900, Masahiro Yamada wrote:
>>> Add two compatible strings for UniPhier SoCs.  The revision register
>>> on both shows revision 5.0, but they are different hardware.
>>>
>>> Features:
>>>  - DMA engine with 64 bit physical address support
>>>  - 1024 byte ECC step size
>>>  - 8 / 16 / 24 bit ECC strength
>>>  - The n_banks format depends on SoC
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
>>> ---
>>>
>>>  .../devicetree/bindings/mtd/denali-nand.txt        | 10 +++++--
>>>  drivers/mtd/nand/denali_dt.c                       | 33 ++++++++++++++++++++--
>>>  2 files changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>>> index 51fe195..cea46e2 100644
>>> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
>>> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>>> @@ -1,13 +1,19 @@
>>>  * Denali NAND controller
>>>
>>>  Required properties:
>>> -  - compatible : should be "denali,denali-nand-dt"
>>> +  - compatible : should be one of the following:
>>> +      "denali,denali-nand-dt"
>>
>> There are multiple things wrong with this string. denali,denali is
>> redundant is one.
>
> One more redundancy; "-dt" is weird because
> DT compatible should be a name of hardware.
>
>
>> It's also fairly useless as this IP has several
>> versions and numerous configuration options IIRC. This should be
>> deprecated IMO.
>
> Right.  There are several customizable parameters for this IP,
> so a generic compatible string like this is probably useless.
>
> This DT binding was added by commit 30f9f2f for Altera SOCFPGA,
> A funny thing is that they upstreamed DT binding, but they did not upstream
> needed changes for the Denali driver core.
> So, the mainline driver has never worked on SOCFPGA
> (or any of DT-based SoCs).
>
>
>
>>> +      "denali,denali-nand-uniphier-v5a"
>>> +      "denali,denali-nand-uniphier-v5b"
>>
>> Use your vendor prefix, not denali. The 2nd denali can probably be
>> dropped because it is not likely you have another kind of nand
>> controller in the SoC.
>
> Hmm, your statement implies that a vendor prefix
> belongs to an SoC vendor, not an IP vendor.
> (I was not quite sure about this.)

Right. We do have some IP vendor strings, but they are usually
fallbacks. Sometimes, I guess they are useful, but IMO over time they
prove to not be useful.

> It is unlikely to happen to have two different NAND controllers on one SoC.
> But, we used a different NAND controller for our SoC family before
> introducing the Denali IP.
> It also implies that Socionext may use a different NAND IP in the future.
> I'd like to include "denali" somewhere because it is clearly associated with
> the driver name.
> Also, this will give an idea what kind of _basic_ hardware is used,
> even though we know various parameters are customizable.
>
>
>
> (Plan A)
>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>
> (Plan B)
>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)

Let the Altera folks worry about their stuff. At least for soft IP in
FPGA, it's a bit of a special case. The old string can remain as bad
as it is. I simply would do "socionext,uniphier-v5b-nand" (and v5a).
The fact that it is denali is part of the documentation.

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

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Hi Rob,

2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:

>>
>>
>> (Plan A)
>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>
>> (Plan B)
>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)

> Let the Altera folks worry about their stuff. At least for soft IP in
> FPGA, it's a bit of a special case. The old string can remain as bad
> as it is.


Hmm, I am not sure if this IP would fit in FPGA
(to use it along with NIOS-II?)

(even if it happened, nothing of this IP would be customizable on users' side.
When buying the IP, SoC vendors submit a list of desired features.
Denali (now Cadence) generates the RTL according to the configuration sheet.
The function is fixed at this point. So, generic compatible would be
useless anyway.)


If we are talking about SOCFPGA,
SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
It consists of two parts:
[1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
USB, SD, NAND, ...)
[2] FPGA part (User design logic)

The Denali NAND controller is included in [1].
So, as far as we talk about the Denali on SOCFPGA,
it is as hard-wired as Intel, Socionext's ones.


> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
> The fact that it is denali is part of the documentation.
>

Let me think about this.

Socionext bought two version of Denali IP,
and we are now re-using the newer one (v5b) for several SoCs.
Socionext has some more product lines other than Uniphier SoC family,
perhaps wider re-use might happen in the future.

At first, I included "uniphier" in compatible, but I am still wondering
if such a specific string is good or not.

Also, comments from Altera engineers are appreciated.


-- 
Best Regards
Masahiro Yamada

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

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Marek Vasut]

On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
> Hi Rob,

Hi!

> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> 
>>>
>>>
>>> (Plan A)
>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>
>>> (Plan B)
>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
> 
>> Let the Altera folks worry about their stuff. At least for soft IP in
>> FPGA, it's a bit of a special case. The old string can remain as bad
>> as it is.
> 
> 
> Hmm, I am not sure if this IP would fit in FPGA
> (to use it along with NIOS-II?)
> 
> (even if it happened, nothing of this IP would be customizable on users' side.
> When buying the IP, SoC vendors submit a list of desired features.
> Denali (now Cadence) generates the RTL according to the configuration sheet.
> The function is fixed at this point. So, generic compatible would be
> useless anyway.)
> 
> 
> If we are talking about SOCFPGA,
> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
> It consists of two parts:
> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
> USB, SD, NAND, ...)
> [2] FPGA part (User design logic)
> 
> The Denali NAND controller is included in [1].
> So, as far as we talk about the Denali on SOCFPGA,
> it is as hard-wired as Intel, Socionext's ones.

That's correct, the Denali NAND IP in altera socfpga is a hardware
block. You can make it available to the fabric too, but by default
it's used by the ARM part of the chip, so for this discussion, you
can forget that the FPGA part exists altogether.

I would be in favor of plan B, since it seems to be the more often
taken approach. A nice example is ci-hdrc:

$ git grep compatible drivers/usb/chipidea/

>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>> The fact that it is denali is part of the documentation.
>>
> 
> Let me think about this.
> 
> Socionext bought two version of Denali IP,
> and we are now re-using the newer one (v5b) for several SoCs.
> Socionext has some more product lines other than Uniphier SoC family,
> perhaps wider re-use might happen in the future.
> 
> At first, I included "uniphier" in compatible, but I am still wondering
> if such a specific string is good or not.
> 
> Also, comments from Altera engineers are appreciated.

Adding a few more on Cc

-- 
Best regards,
Marek Vasut
--
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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Dinh Nguyen]

Hi,

On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>> Hi Rob,
>
> Hi!
>
>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:
>>
>>>>
>>>>
>>>> (Plan A)
>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>
>>>> (Plan B)
>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>
>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>> as it is.
>>
>>
>> Hmm, I am not sure if this IP would fit in FPGA
>> (to use it along with NIOS-II?)
>>
>> (even if it happened, nothing of this IP would be customizable on users' side.
>> When buying the IP, SoC vendors submit a list of desired features.
>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>> The function is fixed at this point. So, generic compatible would be
>> useless anyway.)
>>
>>
>> If we are talking about SOCFPGA,
>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>> It consists of two parts:
>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>> USB, SD, NAND, ...)
>> [2] FPGA part (User design logic)
>>
>> The Denali NAND controller is included in [1].
>> So, as far as we talk about the Denali on SOCFPGA,
>> it is as hard-wired as Intel, Socionext's ones.
>
> That's correct, the Denali NAND IP in altera socfpga is a hardware
> block. You can make it available to the fabric too, but by default
> it's used by the ARM part of the chip, so for this discussion, you
> can forget that the FPGA part exists altogether.
>
> I would be in favor of plan B, since it seems to be the more often
> taken approach. A nice example is ci-hdrc:
>
> $ git grep compatible drivers/usb/chipidea/
>
>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>> The fact that it is denali is part of the documentation.
>>>
>>
>> Let me think about this.
>>
>> Socionext bought two version of Denali IP,
>> and we are now re-using the newer one (v5b) for several SoCs.
>> Socionext has some more product lines other than Uniphier SoC family,
>> perhaps wider re-use might happen in the future.
>>
>> At first, I included "uniphier" in compatible, but I am still wondering
>> if such a specific string is good or not.
>>
>> Also, comments from Altera engineers are appreciated.

Sorry, it's taken me a while to add comments. My altera email is very spotty now
that the Intel merge is completed. Please use dinguyen@kernel.org for any future
communications.

Yes, everything that is said so far for the NAND controller on the
SoCFPGA is correct. I added the binding for the controller a while
back, but unfortunately, we never added the NAND interface to the
devkit, so we did not do much in terms of enabling it.

I think the only SoCFPGA board I know that has the NAND interface active is
the TRCom board, but I have never seen that board.

I don't have any strong opinions on this matter, just as long as the
original binding
"denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
that binding.

Dinh

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Hi Dinh,


2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hi,
>
> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>> Hi Rob,
>>
>> Hi!
>>
>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>>
>>>>>
>>>>>
>>>>> (Plan A)
>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>
>>>>> (Plan B)
>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>
>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>> as it is.
>>>
>>>
>>> Hmm, I am not sure if this IP would fit in FPGA
>>> (to use it along with NIOS-II?)
>>>
>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>> When buying the IP, SoC vendors submit a list of desired features.
>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>> The function is fixed at this point. So, generic compatible would be
>>> useless anyway.)
>>>
>>>
>>> If we are talking about SOCFPGA,
>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>> It consists of two parts:
>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>> USB, SD, NAND, ...)
>>> [2] FPGA part (User design logic)
>>>
>>> The Denali NAND controller is included in [1].
>>> So, as far as we talk about the Denali on SOCFPGA,
>>> it is as hard-wired as Intel, Socionext's ones.
>>
>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>> block. You can make it available to the fabric too, but by default
>> it's used by the ARM part of the chip, so for this discussion, you
>> can forget that the FPGA part exists altogether.
>>
>> I would be in favor of plan B, since it seems to be the more often
>> taken approach. A nice example is ci-hdrc:
>>
>> $ git grep compatible drivers/usb/chipidea/
>>
>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>> The fact that it is denali is part of the documentation.
>>>>
>>>
>>> Let me think about this.
>>>
>>> Socionext bought two version of Denali IP,
>>> and we are now re-using the newer one (v5b) for several SoCs.
>>> Socionext has some more product lines other than Uniphier SoC family,
>>> perhaps wider re-use might happen in the future.
>>>
>>> At first, I included "uniphier" in compatible, but I am still wondering
>>> if such a specific string is good or not.
>>>
>>> Also, comments from Altera engineers are appreciated.
>
> Sorry, it's taken me a while to add comments. My altera email is very spotty now
> that the Intel merge is completed. Please use dinguyen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org for any future
> communications.
>
> Yes, everything that is said so far for the NAND controller on the
> SoCFPGA is correct. I added the binding for the controller a while
> back, but unfortunately, we never added the NAND interface to the
> devkit, so we did not do much in terms of enabling it.
>
> I think the only SoCFPGA board I know that has the NAND interface active is
> the TRCom board, but I have never seen that board.
>
> I don't have any strong opinions on this matter, just as long as the
> original binding
> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
> that binding.
>

I am proposing to add "altera,denali-nand" for Altera.
For what, do you need the generic compatible?
This IP has no default for it to fallback to.


-- 
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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Marek Vasut]

On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
> Hi Dinh,
> 
> 
> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> Hi,
>>
>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>> Hi Rob,
>>>
>>> Hi!
>>>
>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>>>
>>>>>>
>>>>>>
>>>>>> (Plan A)
>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>
>>>>>> (Plan B)
>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>
>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>> as it is.
>>>>
>>>>
>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>> (to use it along with NIOS-II?)
>>>>
>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>> The function is fixed at this point. So, generic compatible would be
>>>> useless anyway.)
>>>>
>>>>
>>>> If we are talking about SOCFPGA,
>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>> It consists of two parts:
>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>> USB, SD, NAND, ...)
>>>> [2] FPGA part (User design logic)
>>>>
>>>> The Denali NAND controller is included in [1].
>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>> it is as hard-wired as Intel, Socionext's ones.
>>>
>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>> block. You can make it available to the fabric too, but by default
>>> it's used by the ARM part of the chip, so for this discussion, you
>>> can forget that the FPGA part exists altogether.
>>>
>>> I would be in favor of plan B, since it seems to be the more often
>>> taken approach. A nice example is ci-hdrc:
>>>
>>> $ git grep compatible drivers/usb/chipidea/
>>>
>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>> The fact that it is denali is part of the documentation.
>>>>>
>>>>
>>>> Let me think about this.
>>>>
>>>> Socionext bought two version of Denali IP,
>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>> perhaps wider re-use might happen in the future.
>>>>
>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>> if such a specific string is good or not.
>>>>
>>>> Also, comments from Altera engineers are appreciated.
>>
>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>> that the Intel merge is completed. Please use dinguyen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org for any future
>> communications.
>>
>> Yes, everything that is said so far for the NAND controller on the
>> SoCFPGA is correct. I added the binding for the controller a while
>> back, but unfortunately, we never added the NAND interface to the
>> devkit, so we did not do much in terms of enabling it.
>>
>> I think the only SoCFPGA board I know that has the NAND interface active is
>> the TRCom board, but I have never seen that board.
>>
>> I don't have any strong opinions on this matter, just as long as the
>> original binding
>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>> that binding.
>>
> 
> I am proposing to add "altera,denali-nand" for Altera.
> For what, do you need the generic compatible?
> This IP has no default for it to fallback to.

IMO just for compatibility reasons with old DTs .
I'm also for "altera,denali-nand" or maybe "altera,socfpga-denali-nand"
to be more precise ?

-- 
Best regards,
Marek Vasut
--
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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Masahiro Yamada]

Hi Marek,


2016-12-05 12:44 GMT+09:00 Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
>> Hi Dinh,
>>
>>
>> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>>> Hi,
>>>
>>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>>> Hi Rob,
>>>>
>>>> Hi!
>>>>
>>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> (Plan A)
>>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>>
>>>>>>> (Plan B)
>>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>>
>>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>>> as it is.
>>>>>
>>>>>
>>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>>> (to use it along with NIOS-II?)
>>>>>
>>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>>> The function is fixed at this point. So, generic compatible would be
>>>>> useless anyway.)
>>>>>
>>>>>
>>>>> If we are talking about SOCFPGA,
>>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>>> It consists of two parts:
>>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>>> USB, SD, NAND, ...)
>>>>> [2] FPGA part (User design logic)
>>>>>
>>>>> The Denali NAND controller is included in [1].
>>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>>> it is as hard-wired as Intel, Socionext's ones.
>>>>
>>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>>> block. You can make it available to the fabric too, but by default
>>>> it's used by the ARM part of the chip, so for this discussion, you
>>>> can forget that the FPGA part exists altogether.
>>>>
>>>> I would be in favor of plan B, since it seems to be the more often
>>>> taken approach. A nice example is ci-hdrc:
>>>>
>>>> $ git grep compatible drivers/usb/chipidea/
>>>>
>>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>>> The fact that it is denali is part of the documentation.
>>>>>>
>>>>>
>>>>> Let me think about this.
>>>>>
>>>>> Socionext bought two version of Denali IP,
>>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>>> perhaps wider re-use might happen in the future.
>>>>>
>>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>>> if such a specific string is good or not.
>>>>>
>>>>> Also, comments from Altera engineers are appreciated.
>>>
>>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>>> that the Intel merge is completed. Please use dinguyen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org for any future
>>> communications.
>>>
>>> Yes, everything that is said so far for the NAND controller on the
>>> SoCFPGA is correct. I added the binding for the controller a while
>>> back, but unfortunately, we never added the NAND interface to the
>>> devkit, so we did not do much in terms of enabling it.
>>>
>>> I think the only SoCFPGA board I know that has the NAND interface active is
>>> the TRCom board, but I have never seen that board.
>>>
>>> I don't have any strong opinions on this matter, just as long as the
>>> original binding
>>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>>> that binding.
>>>
>>
>> I am proposing to add "altera,denali-nand" for Altera.
>> For what, do you need the generic compatible?
>> This IP has no default for it to fallback to.
>
> IMO just for compatibility reasons with old DTs .

We generally contribute for
a "working driver" (at least, should be functional to some extent)
and "DT binding" bundled together.

However, Altera upstreamed the DT binding first
(then some parts of the DT binding turned out wrong),
but they did not upstream needed driver changes in the end.

So, the mainline driver has never worked on SOCFPGA, right?
Removing "denali,denali-nand-dt" is not breakage at all,
so I do not owe anything to them, right?



-- 
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 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Marek Vasut]

On 12/05/2016 05:10 AM, Masahiro Yamada wrote:
> Hi Marek,
> 
> 
> 2016-12-05 12:44 GMT+09:00 Marek Vasut <marek.vasut@gmail.com>:
>> On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
>>> Hi Dinh,
>>>
>>>
>>> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux@gmail.com>:
>>>> Hi,
>>>>
>>>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>>>> Hi Rob,
>>>>>
>>>>> Hi!
>>>>>
>>>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> (Plan A)
>>>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>>>
>>>>>>>> (Plan B)
>>>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>>>
>>>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>>>> as it is.
>>>>>>
>>>>>>
>>>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>>>> (to use it along with NIOS-II?)
>>>>>>
>>>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>>>> The function is fixed at this point. So, generic compatible would be
>>>>>> useless anyway.)
>>>>>>
>>>>>>
>>>>>> If we are talking about SOCFPGA,
>>>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>>>> It consists of two parts:
>>>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>>>> USB, SD, NAND, ...)
>>>>>> [2] FPGA part (User design logic)
>>>>>>
>>>>>> The Denali NAND controller is included in [1].
>>>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>>>> it is as hard-wired as Intel, Socionext's ones.
>>>>>
>>>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>>>> block. You can make it available to the fabric too, but by default
>>>>> it's used by the ARM part of the chip, so for this discussion, you
>>>>> can forget that the FPGA part exists altogether.
>>>>>
>>>>> I would be in favor of plan B, since it seems to be the more often
>>>>> taken approach. A nice example is ci-hdrc:
>>>>>
>>>>> $ git grep compatible drivers/usb/chipidea/
>>>>>
>>>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>>>> The fact that it is denali is part of the documentation.
>>>>>>>
>>>>>>
>>>>>> Let me think about this.
>>>>>>
>>>>>> Socionext bought two version of Denali IP,
>>>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>>>> perhaps wider re-use might happen in the future.
>>>>>>
>>>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>>>> if such a specific string is good or not.
>>>>>>
>>>>>> Also, comments from Altera engineers are appreciated.
>>>>
>>>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>>>> that the Intel merge is completed. Please use dinguyen@kernel.org for any future
>>>> communications.
>>>>
>>>> Yes, everything that is said so far for the NAND controller on the
>>>> SoCFPGA is correct. I added the binding for the controller a while
>>>> back, but unfortunately, we never added the NAND interface to the
>>>> devkit, so we did not do much in terms of enabling it.
>>>>
>>>> I think the only SoCFPGA board I know that has the NAND interface active is
>>>> the TRCom board, but I have never seen that board.
>>>>
>>>> I don't have any strong opinions on this matter, just as long as the
>>>> original binding
>>>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>>>> that binding.
>>>>
>>>
>>> I am proposing to add "altera,denali-nand" for Altera.
>>> For what, do you need the generic compatible?
>>> This IP has no default for it to fallback to.
>>
>> IMO just for compatibility reasons with old DTs .
> 
> We generally contribute for
> a "working driver" (at least, should be functional to some extent)
> and "DT binding" bundled together.
> 
> However, Altera upstreamed the DT binding first
> (then some parts of the DT binding turned out wrong),
> but they did not upstream needed driver changes in the end.
> 
> So, the mainline driver has never worked on SOCFPGA, right?

Most likely it never worked, yes.

> Removing "denali,denali-nand-dt" is not breakage at all,
> so I do not owe anything to them, right?

I don't think I'm really qualified to answer this one. But, there is
drivers/mtd/nand/denali_dt.c , which handles this compatible string
and it's documented in
Documentation/devicetree/bindings/mtd/denali-nand.txt, so doesn't that
make it part of the ABI ? I think we should
at least keep it as a fallback, that should be pretty harmless.

-- 
Best regards,
Marek Vasut

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Dinh Nguyen]

On Sun, Dec 4, 2016 at 10:22 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/05/2016 05:10 AM, Masahiro Yamada wrote:
>> Hi Marek,
>>
>>
>> 2016-12-05 12:44 GMT+09:00 Marek Vasut <marek.vasut@gmail.com>:
>>> On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
>>>> Hi Dinh,
>>>>
>>>>
>>>> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux@gmail.com>:
>>>>> Hi,
>>>>>
>>>>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>>>>> Hi Rob,
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> (Plan A)
>>>>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>>>>
>>>>>>>>> (Plan B)
>>>>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>>>>
>>>>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>>>>> as it is.
>>>>>>>
>>>>>>>
>>>>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>>>>> (to use it along with NIOS-II?)
>>>>>>>
>>>>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>>>>> The function is fixed at this point. So, generic compatible would be
>>>>>>> useless anyway.)
>>>>>>>
>>>>>>>
>>>>>>> If we are talking about SOCFPGA,
>>>>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>>>>> It consists of two parts:
>>>>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>>>>> USB, SD, NAND, ...)
>>>>>>> [2] FPGA part (User design logic)
>>>>>>>
>>>>>>> The Denali NAND controller is included in [1].
>>>>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>>>>> it is as hard-wired as Intel, Socionext's ones.
>>>>>>
>>>>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>>>>> block. You can make it available to the fabric too, but by default
>>>>>> it's used by the ARM part of the chip, so for this discussion, you
>>>>>> can forget that the FPGA part exists altogether.
>>>>>>
>>>>>> I would be in favor of plan B, since it seems to be the more often
>>>>>> taken approach. A nice example is ci-hdrc:
>>>>>>
>>>>>> $ git grep compatible drivers/usb/chipidea/
>>>>>>
>>>>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>>>>> The fact that it is denali is part of the documentation.
>>>>>>>>
>>>>>>>
>>>>>>> Let me think about this.
>>>>>>>
>>>>>>> Socionext bought two version of Denali IP,
>>>>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>>>>> perhaps wider re-use might happen in the future.
>>>>>>>
>>>>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>>>>> if such a specific string is good or not.
>>>>>>>
>>>>>>> Also, comments from Altera engineers are appreciated.
>>>>>
>>>>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>>>>> that the Intel merge is completed. Please use dinguyen@kernel.org for any future
>>>>> communications.
>>>>>
>>>>> Yes, everything that is said so far for the NAND controller on the
>>>>> SoCFPGA is correct. I added the binding for the controller a while
>>>>> back, but unfortunately, we never added the NAND interface to the
>>>>> devkit, so we did not do much in terms of enabling it.
>>>>>
>>>>> I think the only SoCFPGA board I know that has the NAND interface active is
>>>>> the TRCom board, but I have never seen that board.
>>>>>
>>>>> I don't have any strong opinions on this matter, just as long as the
>>>>> original binding
>>>>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>>>>> that binding.
>>>>>
>>>>
>>>> I am proposing to add "altera,denali-nand" for Altera.
>>>> For what, do you need the generic compatible?
>>>> This IP has no default for it to fallback to.
>>>
>>> IMO just for compatibility reasons with old DTs .
>>
>> We generally contribute for
>> a "working driver" (at least, should be functional to some extent)
>> and "DT binding" bundled together.
>>
>> However, Altera upstreamed the DT binding first
>> (then some parts of the DT binding turned out wrong),
>> but they did not upstream needed driver changes in the end.
>>
>> So, the mainline driver has never worked on SOCFPGA, right?
>
> Most likely it never worked, yes.
>

Right, looking through our downstream support, we may need to upstream a
few changes to make upstream driver work on SoCFPGA.

>> Removing "denali,denali-nand-dt" is not breakage at all,
>> so I do not owe anything to them, right?
>
> I don't think I'm really qualified to answer this one. But, there is
> drivers/mtd/nand/denali_dt.c , which handles this compatible string
> and it's documented in
> Documentation/devicetree/bindings/mtd/denali-nand.txt, so doesn't that
> make it part of the ABI ? I think we should
> at least keep it as a fallback, that should be pretty harmless.
>

I would like to propose "altr,denali-nand" as the binding we use to support the
driver going forward on SoCFPGA hardware. It's pretty much the same as
"altera,denali-nand", just with the correct vendor prefix.

If we can please keep, "denali,denali-nand-dt" only because SoCFPGA is using
this binding downstream, but I know that is a weak argument.

Dinh

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Marek Vasut]

On 12/05/2016 09:51 PM, Dinh Nguyen wrote:
> On Sun, Dec 4, 2016 at 10:22 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 12/05/2016 05:10 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>>
>>>
>>> 2016-12-05 12:44 GMT+09:00 Marek Vasut <marek.vasut@gmail.com>:
>>>> On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
>>>>> Hi Dinh,
>>>>>
>>>>>
>>>>> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux@gmail.com>:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (Plan A)
>>>>>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>>>>>
>>>>>>>>>> (Plan B)
>>>>>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>>>>>
>>>>>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>>>>>> as it is.
>>>>>>>>
>>>>>>>>
>>>>>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>>>>>> (to use it along with NIOS-II?)
>>>>>>>>
>>>>>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>>>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>>>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>>>>>> The function is fixed at this point. So, generic compatible would be
>>>>>>>> useless anyway.)
>>>>>>>>
>>>>>>>>
>>>>>>>> If we are talking about SOCFPGA,
>>>>>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>>>>>> It consists of two parts:
>>>>>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>>>>>> USB, SD, NAND, ...)
>>>>>>>> [2] FPGA part (User design logic)
>>>>>>>>
>>>>>>>> The Denali NAND controller is included in [1].
>>>>>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>>>>>> it is as hard-wired as Intel, Socionext's ones.
>>>>>>>
>>>>>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>>>>>> block. You can make it available to the fabric too, but by default
>>>>>>> it's used by the ARM part of the chip, so for this discussion, you
>>>>>>> can forget that the FPGA part exists altogether.
>>>>>>>
>>>>>>> I would be in favor of plan B, since it seems to be the more often
>>>>>>> taken approach. A nice example is ci-hdrc:
>>>>>>>
>>>>>>> $ git grep compatible drivers/usb/chipidea/
>>>>>>>
>>>>>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>>>>>> The fact that it is denali is part of the documentation.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Let me think about this.
>>>>>>>>
>>>>>>>> Socionext bought two version of Denali IP,
>>>>>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>>>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>>>>>> perhaps wider re-use might happen in the future.
>>>>>>>>
>>>>>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>>>>>> if such a specific string is good or not.
>>>>>>>>
>>>>>>>> Also, comments from Altera engineers are appreciated.
>>>>>>
>>>>>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>>>>>> that the Intel merge is completed. Please use dinguyen@kernel.org for any future
>>>>>> communications.
>>>>>>
>>>>>> Yes, everything that is said so far for the NAND controller on the
>>>>>> SoCFPGA is correct. I added the binding for the controller a while
>>>>>> back, but unfortunately, we never added the NAND interface to the
>>>>>> devkit, so we did not do much in terms of enabling it.
>>>>>>
>>>>>> I think the only SoCFPGA board I know that has the NAND interface active is
>>>>>> the TRCom board, but I have never seen that board.
>>>>>>
>>>>>> I don't have any strong opinions on this matter, just as long as the
>>>>>> original binding
>>>>>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>>>>>> that binding.
>>>>>>
>>>>>
>>>>> I am proposing to add "altera,denali-nand" for Altera.
>>>>> For what, do you need the generic compatible?
>>>>> This IP has no default for it to fallback to.
>>>>
>>>> IMO just for compatibility reasons with old DTs .
>>>
>>> We generally contribute for
>>> a "working driver" (at least, should be functional to some extent)
>>> and "DT binding" bundled together.
>>>
>>> However, Altera upstreamed the DT binding first
>>> (then some parts of the DT binding turned out wrong),
>>> but they did not upstream needed driver changes in the end.
>>>
>>> So, the mainline driver has never worked on SOCFPGA, right?
>>
>> Most likely it never worked, yes.
>>
> 
> Right, looking through our downstream support, we may need to upstream a
> few changes to make upstream driver work on SoCFPGA.
> 
>>> Removing "denali,denali-nand-dt" is not breakage at all,
>>> so I do not owe anything to them, right?
>>
>> I don't think I'm really qualified to answer this one. But, there is
>> drivers/mtd/nand/denali_dt.c , which handles this compatible string
>> and it's documented in
>> Documentation/devicetree/bindings/mtd/denali-nand.txt, so doesn't that
>> make it part of the ABI ? I think we should
>> at least keep it as a fallback, that should be pretty harmless.
>>
> 
> I would like to propose "altr,denali-nand" as the binding we use to support the
> driver going forward on SoCFPGA hardware. It's pretty much the same as
> "altera,denali-nand", just with the correct vendor prefix.

Ah right, altr is the right prefix, thanks for pointing that out.
Still, wouldn't altr,socfpga-denali-nand be better ? I know it's
long, but it encodes the chip type , like ie. fsl,imx6q-usb .

> If we can please keep, "denali,denali-nand-dt" only because SoCFPGA is using
> this binding downstream, but I know that is a weak argument.

-- 
Best regards,
Marek Vasut

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

Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

[Dinh Nguyen]

On 12/05/2016 03:29 PM, Marek Vasut wrote:
> On 12/05/2016 09:51 PM, Dinh Nguyen wrote:
>> On Sun, Dec 4, 2016 at 10:22 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 12/05/2016 05:10 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>>
>>>>
>>>> 2016-12-05 12:44 GMT+09:00 Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>>>>> On 12/05/2016 04:30 AM, Masahiro Yamada wrote:
>>>>>> Hi Dinh,
>>>>>>
>>>>>>
>>>>>> 2016-12-04 7:08 GMT+09:00 Dinh Nguyen <dinh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Fri, Dec 2, 2016 at 8:49 PM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>> On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
>>>>>>>>> Hi Rob,
>>>>>>>>
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>>> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> (Plan A)
>>>>>>>>>>>   "denali,socfpga-nand"           (for Altera SOCFPGA variant)
>>>>>>>>>>>   "denali,uniphier-nand-v1"       (for old Socionext UniPhier family variant)
>>>>>>>>>>>   "denali,uniphier-nand-v2"       (for new Socionext UniPhier family variant)
>>>>>>>>>>>
>>>>>>>>>>> (Plan B)
>>>>>>>>>>>   "altera,denali-nand"            (for Altera SOCFPGA variant)
>>>>>>>>>>>   "socionext,denali-nand-v5a"     (for old Socionext UniPhier family variant)
>>>>>>>>>>>   "socionext,denali-nand-v5b"     (for new Socionext UniPhier family variant)
>>>>>>>>>
>>>>>>>>>> Let the Altera folks worry about their stuff. At least for soft IP in
>>>>>>>>>> FPGA, it's a bit of a special case. The old string can remain as bad
>>>>>>>>>> as it is.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hmm, I am not sure if this IP would fit in FPGA
>>>>>>>>> (to use it along with NIOS-II?)
>>>>>>>>>
>>>>>>>>> (even if it happened, nothing of this IP would be customizable on users' side.
>>>>>>>>> When buying the IP, SoC vendors submit a list of desired features.
>>>>>>>>> Denali (now Cadence) generates the RTL according to the configuration sheet.
>>>>>>>>> The function is fixed at this point. So, generic compatible would be
>>>>>>>>> useless anyway.)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If we are talking about SOCFPGA,
>>>>>>>>> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
>>>>>>>>> It consists of two parts:
>>>>>>>>> [1] SOC part  (Cortex-A9 + various hard-wired peripherals such UART,
>>>>>>>>> USB, SD, NAND, ...)
>>>>>>>>> [2] FPGA part (User design logic)
>>>>>>>>>
>>>>>>>>> The Denali NAND controller is included in [1].
>>>>>>>>> So, as far as we talk about the Denali on SOCFPGA,
>>>>>>>>> it is as hard-wired as Intel, Socionext's ones.
>>>>>>>>
>>>>>>>> That's correct, the Denali NAND IP in altera socfpga is a hardware
>>>>>>>> block. You can make it available to the fabric too, but by default
>>>>>>>> it's used by the ARM part of the chip, so for this discussion, you
>>>>>>>> can forget that the FPGA part exists altogether.
>>>>>>>>
>>>>>>>> I would be in favor of plan B, since it seems to be the more often
>>>>>>>> taken approach. A nice example is ci-hdrc:
>>>>>>>>
>>>>>>>> $ git grep compatible drivers/usb/chipidea/
>>>>>>>>
>>>>>>>>>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>>>>>>>>>> The fact that it is denali is part of the documentation.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Let me think about this.
>>>>>>>>>
>>>>>>>>> Socionext bought two version of Denali IP,
>>>>>>>>> and we are now re-using the newer one (v5b) for several SoCs.
>>>>>>>>> Socionext has some more product lines other than Uniphier SoC family,
>>>>>>>>> perhaps wider re-use might happen in the future.
>>>>>>>>>
>>>>>>>>> At first, I included "uniphier" in compatible, but I am still wondering
>>>>>>>>> if such a specific string is good or not.
>>>>>>>>>
>>>>>>>>> Also, comments from Altera engineers are appreciated.
>>>>>>>
>>>>>>> Sorry, it's taken me a while to add comments. My altera email is very spotty now
>>>>>>> that the Intel merge is completed. Please use dinguyen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org for any future
>>>>>>> communications.
>>>>>>>
>>>>>>> Yes, everything that is said so far for the NAND controller on the
>>>>>>> SoCFPGA is correct. I added the binding for the controller a while
>>>>>>> back, but unfortunately, we never added the NAND interface to the
>>>>>>> devkit, so we did not do much in terms of enabling it.
>>>>>>>
>>>>>>> I think the only SoCFPGA board I know that has the NAND interface active is
>>>>>>> the TRCom board, but I have never seen that board.
>>>>>>>
>>>>>>> I don't have any strong opinions on this matter, just as long as the
>>>>>>> original binding
>>>>>>> "denali,denali-nand-dt" is kept, and I think Rob was ok with keeping
>>>>>>> that binding.
>>>>>>>
>>>>>>
>>>>>> I am proposing to add "altera,denali-nand" for Altera.
>>>>>> For what, do you need the generic compatible?
>>>>>> This IP has no default for it to fallback to.
>>>>>
>>>>> IMO just for compatibility reasons with old DTs .
>>>>
>>>> We generally contribute for
>>>> a "working driver" (at least, should be functional to some extent)
>>>> and "DT binding" bundled together.
>>>>
>>>> However, Altera upstreamed the DT binding first
>>>> (then some parts of the DT binding turned out wrong),
>>>> but they did not upstream needed driver changes in the end.
>>>>
>>>> So, the mainline driver has never worked on SOCFPGA, right?
>>>
>>> Most likely it never worked, yes.
>>>
>>
>> Right, looking through our downstream support, we may need to upstream a
>> few changes to make upstream driver work on SoCFPGA.
>>
>>>> Removing "denali,denali-nand-dt" is not breakage at all,
>>>> so I do not owe anything to them, right?
>>>
>>> I don't think I'm really qualified to answer this one. But, there is
>>> drivers/mtd/nand/denali_dt.c , which handles this compatible string
>>> and it's documented in
>>> Documentation/devicetree/bindings/mtd/denali-nand.txt, so doesn't that
>>> make it part of the ABI ? I think we should
>>> at least keep it as a fallback, that should be pretty harmless.
>>>
>>
>> I would like to propose "altr,denali-nand" as the binding we use to support the
>> driver going forward on SoCFPGA hardware. It's pretty much the same as
>> "altera,denali-nand", just with the correct vendor prefix.
> 
> Ah right, altr is the right prefix, thanks for pointing that out.
> Still, wouldn't altr,socfpga-denali-nand be better ? I know it's
> long, but it encodes the chip type , like ie. fsl,imx6q-usb .
> 

Yes, that's fine.

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

[Masahiro Yamada]

Hi Boris,

I am almost getting v2 done,
and now I am testing it.

I am having one problem.  Please teach me.


2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>> [2]
>> Remove driver-internal bounce buffer.
>> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
>> to use it as a driver-internal bounce buffer.
>>
>> The hardware transfer page data into the bounce buffer,
>> then CPU copies from the bounce buffer to a given buf (and oob_poi).
>> This is not efficient.
>>
>> So, I want to set NAND_USE_BOUNCE_BUFFER flag
>> and do dma_map_single directly for a given buffer.
>
> Sounds good. Be careful though, when you use the generic bounce buffer
> interface you might have to clear the page cache info (->pagebuf = -1).


Instead of memcpy() of the whole page,
I am trying to use dma_map_single()  in ecc->read_page() / ecc->write_page().
This will allow direct transfer between the buffer and the device by DMA.

But, this does not work for Denali if use_bufpoi is set in nand_do_read_ops().


In the following code in nand_scan_tail(),

        if (!(chip->options & NAND_OWN_BUFFERS)) {
                nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
                                + mtd->oobsize * 3, GFP_KERNEL);
                if (!nbuf)
                        return -ENOMEM;
                nbuf->ecccalc = (uint8_t *)(nbuf + 1);
                nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
                nbuf->databuf = nbuf->ecccode + mtd->oobsize;

                chip->buffers = nbuf;


chip->buffers->databuf has no guarantee for DMA'able alignment.
(actually it has unwanted offset 0xc because sizeof(*nbuf) == 0xc on
32bit systems)

If we could change the code as follows,

                nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
                nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
                nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
GFP_KERNEL);

chip->buffers->databuf would have DMA'able alignment in most cases
without NAND_OWN_BUFFERS.  (but, I am not sure if this is a good idea)


So, the idea of NAND_OWN_BUFFERS is that
drivers should allocate own buffers if they need to perform DMA-mapping
in read_page(), write_page(), right?


However, "git grep NAND_OWN_BUFFERS" shows
cafe_nand.c is the only driver that does so.

On the other hand, "git grep dma_map_single" has more hits,
i.e. some drivers perform dma_map_single() for read/write without
NAND_OWN_BUFFERS.

I have no idea how they are working.



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

[Boris Brezillon]

Hi Masahiro,

On Fri, 10 Mar 2017 20:00:03 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:

> Hi Boris,
> 
> I am almost getting v2 done,
> and now I am testing it.
> 
> I am having one problem.  Please teach me.
> 
> 
> 2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >> [2]
> >> Remove driver-internal bounce buffer.
> >> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
> >> to use it as a driver-internal bounce buffer.
> >>
> >> The hardware transfer page data into the bounce buffer,
> >> then CPU copies from the bounce buffer to a given buf (and oob_poi).
> >> This is not efficient.
> >>
> >> So, I want to set NAND_USE_BOUNCE_BUFFER flag
> >> and do dma_map_single directly for a given buffer.  
> >
> > Sounds good. Be careful though, when you use the generic bounce buffer
> > interface you might have to clear the page cache info (->pagebuf = -1).  
> 
> 
> Instead of memcpy() of the whole page,
> I am trying to use dma_map_single()  in ecc->read_page() / ecc->write_page().
> This will allow direct transfer between the buffer and the device by DMA.
> 
> But, this does not work for Denali if use_bufpoi is set in nand_do_read_ops().
> 
> 
> In the following code in nand_scan_tail(),
> 
>         if (!(chip->options & NAND_OWN_BUFFERS)) {
>                 nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
>                                 + mtd->oobsize * 3, GFP_KERNEL);
>                 if (!nbuf)
>                         return -ENOMEM;
>                 nbuf->ecccalc = (uint8_t *)(nbuf + 1);
>                 nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
>                 nbuf->databuf = nbuf->ecccode + mtd->oobsize;
> 
>                 chip->buffers = nbuf;
> 
> 
> chip->buffers->databuf has no guarantee for DMA'able alignment.
> (actually it has unwanted offset 0xc because sizeof(*nbuf) == 0xc on
> 32bit systems)

Well, I think the DMA alignment requirement is a platform/controller
specific (some controllers are fine with this 32bits alignment), but I
get your point.

> 
> If we could change the code as follows,
> 
>                 nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
>                 nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
>                 nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
> GFP_KERNEL);
> 
> chip->buffers->databuf would have DMA'able alignment in most cases
> without NAND_OWN_BUFFERS.  (but, I am not sure if this is a good idea)

I'm fine with this change. I don't know what are the guarantees in term
of alignment when you use kmalloc, but I guess the size you're
allocating (writesize + oobsize) kind of guarantees that the alignment
is rather big (because the SLAB caches are organized by power-of-2
chunk sizes, and for allocations >PAGE_SIZE the page allocator will be
used).

> 
> 
> So, the idea of NAND_OWN_BUFFERS is that
> drivers should allocate own buffers if they need to perform DMA-mapping
> in read_page(), write_page(), right?

Right.

> 
> 
> However, "git grep NAND_OWN_BUFFERS" shows
> cafe_nand.c is the only driver that does so.
> 
> On the other hand, "git grep dma_map_single" has more hits,
> i.e. some drivers perform dma_map_single() for read/write without
> NAND_OWN_BUFFERS.
> 
> I have no idea how they are working.

Probably because the controllers and/or DMA engines have no alignment
constraints.

Anyway, the change you're proposing is rather simple, so go ahead.

Regards,

Boris

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