SPI-NAND support

From: Boris Brezillon <[email protected]>

Some MTD sublayers/drivers are implementing ->_read/write_oob() and provide dummy wrappers for their ->_read/write() implementations. Let the core handle this case instead of duplicating the logic.

Signed-off-by Boris Brezillon <[email protected]>

Acked-by Robert Jarzmik <[email protected]>

Acked-by Brian Norris <[email protected]>

Reviewed-by Miquel Raynal <[email protected]>

Tested-by Ladislav Michl <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---

From: Brian Norris <[email protected]>

We are going to begin using the mtd->dev.of_node field for MTD device nodes, so let's add helpers for it. Also, we'll be making some conversions on spi_nor (and nand_chip eventually) too, so get that ready with their own helpers.

Signed-off-by Brian Norris <[email protected]>

Reviewed-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---

Fix build errors produced by mtd.h and dm/device.h if not included in the right order.

Signed-off-by Miquel Raynal <[email protected]>

---

There is no reason to have NAND, SPI flashes and UBI sections outside of the MTD submenu in Kconfig.

Signed-off-by Miquel Raynal <[email protected]>

---

From: Boris Brezillon <[email protected]>

Add an intermediate layer to abstract NAND device interface so that some logic can be shared between SPI NANDs, parallel/raw NANDs, OneNANDs, ...

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/mtd/nand/Kconfig  |   3 +
 drivers/mtd/nand/Makefile |   3 +
 drivers/mtd/nand/bbt.c    | 132 +++++++++
 drivers/mtd/nand/core.c   | 243 +++++++++++++++
 include/linux/mtd/nand.h  | 731 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1112 insertions(+)
 create mode 100644 drivers/mtd/nand/bbt.c
 create mode 100644 drivers/mtd/nand/core.c
 create mode 100644 include/linux/mtd/nand.h

drivers/mtd/nand/Kconfig +3 -0

index 6d53734718..1c1a1f487e 100644
@@ -1 +1,4 @@
+config MTD_NAND_CORE
+       tristate
+
 source "drivers/mtd/nand/raw/Kconfig"

drivers/mtd/nand/Makefile +3 -0

index d1c3f93047..69c80ea252 100644
@@ -1,3 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
+nandcore-objs := core.o bbt.o
+obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
+
 obj-$(CONFIG_MTD_NAND) += raw/

drivers/mtd/nand/bbt.c new file +28 -0

new file mode 100644
index 0000000000..7e0ad3190c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017 Free Electrons
+ *
+ * Authors:
+ *     Boris Brezillon <[email protected]>
+ *     Peter Pan <[email protected]>
+ */
+
+#define pr_fmt(fmt)    "nand-bbt: " fmt
+
+#include <linux/mtd/nand.h>
+#ifndef __UBOOT__
+#include <linux/slab.h>
+#endif
+
+/**
+ * nanddev_bbt_init() - Initialize the BBT (Bad Block Table)
+ * @nand: NAND device
+ *
+ * Initialize the in-memory BBT.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int nanddev_bbt_init(struct nand_device *nand)
+{
+       unsigned int bits_per_block = fls(NAND_BBT_BLOCK_NUM_STATUS);
+       unsigned int nblocks = nanddev_neraseblocks(nand);

Quoted 77 lines · Miquel Raynal

From: Boris Brezillon <[email protected]>

Add an intermediate layer to abstract NAND device interface so that some logic can be shared between SPI NANDs, parallel/raw NANDs, OneNANDs, ...

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/mtd/nand/Kconfig  |   3 +
 drivers/mtd/nand/Makefile |   3 +
 drivers/mtd/nand/bbt.c    | 132 +++++++++
 drivers/mtd/nand/core.c   | 243 +++++++++++++++
 include/linux/mtd/nand.h  | 731 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1112 insertions(+)
 create mode 100644 drivers/mtd/nand/bbt.c
 create mode 100644 drivers/mtd/nand/core.c
 create mode 100644 include/linux/mtd/nand.h

drivers/mtd/nand/Kconfig +3 -0

index 6d53734718..1c1a1f487e 100644
@@ -1 +1,4 @@
+config MTD_NAND_CORE
+       tristate
+
 source "drivers/mtd/nand/raw/Kconfig"

drivers/mtd/nand/Makefile +3 -0

index d1c3f93047..69c80ea252 100644
@@ -1,3 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
+nandcore-objs := core.o bbt.o
+obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
+
 obj-$(CONFIG_MTD_NAND) += raw/

drivers/mtd/nand/bbt.c new file +32 -0

new file mode 100644
index 0000000000..7e0ad3190c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017 Free Electrons
+ *
+ * Authors:
+ *     Boris Brezillon <[email protected]>
+ *     Peter Pan <[email protected]>
+ */
+
+#define pr_fmt(fmt)    "nand-bbt: " fmt
+
+#include <linux/mtd/nand.h>
+#ifndef __UBOOT__
+#include <linux/slab.h>
+#endif
+
+/**
+ * nanddev_bbt_init() - Initialize the BBT (Bad Block Table)
+ * @nand: NAND device
+ *
+ * Initialize the in-memory BBT.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int nanddev_bbt_init(struct nand_device *nand)
+{
+       unsigned int bits_per_block = fls(NAND_BBT_BLOCK_NUM_STATUS);
+       unsigned int nblocks = nanddev_neraseblocks(nand);
+       unsigned int nwords = DIV_ROUND_UP(nblocks * bits_per_block,
+                                          BITS_PER_LONG);
+
+       nand->bbt.cache = kzalloc(nwords, GFP_KERNEL);

drivers/mtd/nand/bbt.c line 38

Quoted 6 lines

drivers/mtd/nand/bbt.c new file +6 -0

+       if (!nand->bbt.cache)
+               return -ENOMEM;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(nanddev_bbt_init);  

Can't we skip __UBOOT__ and EXPORT_SYMBOL_GPL ?

Hi Jagan,

Quoted 78 lines · Jagan Teki

Quoted 77 lines · Miquel Raynal

From: Boris Brezillon <[email protected]>

Add an intermediate layer to abstract NAND device interface so that some logic can be shared between SPI NANDs, parallel/raw NANDs, OneNANDs, ...

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/mtd/nand/Kconfig  |   3 +
 drivers/mtd/nand/Makefile |   3 +
 drivers/mtd/nand/bbt.c    | 132 +++++++++
 drivers/mtd/nand/core.c   | 243 +++++++++++++++
 include/linux/mtd/nand.h  | 731 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1112 insertions(+)
 create mode 100644 drivers/mtd/nand/bbt.c
 create mode 100644 drivers/mtd/nand/core.c
 create mode 100644 include/linux/mtd/nand.h

drivers/mtd/nand/Kconfig +3 -0

index 6d53734718..1c1a1f487e 100644
@@ -1 +1,4 @@
+config MTD_NAND_CORE
+       tristate
+
 source "drivers/mtd/nand/raw/Kconfig"

drivers/mtd/nand/Makefile +3 -0

index d1c3f93047..69c80ea252 100644
@@ -1,3 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
+nandcore-objs := core.o bbt.o
+obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
+
 obj-$(CONFIG_MTD_NAND) += raw/

drivers/mtd/nand/bbt.c new file +32 -0

new file mode 100644
index 0000000000..7e0ad3190c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017 Free Electrons
+ *
+ * Authors:
+ *     Boris Brezillon <[email protected]>
+ *     Peter Pan <[email protected]>
+ */
+
+#define pr_fmt(fmt)    "nand-bbt: " fmt
+
+#include <linux/mtd/nand.h>
+#ifndef __UBOOT__
+#include <linux/slab.h>
+#endif
+
+/**
+ * nanddev_bbt_init() - Initialize the BBT (Bad Block Table)
+ * @nand: NAND device
+ *
+ * Initialize the in-memory BBT.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int nanddev_bbt_init(struct nand_device *nand)
+{
+       unsigned int bits_per_block = fls(NAND_BBT_BLOCK_NUM_STATUS);
+       unsigned int nblocks = nanddev_neraseblocks(nand);
+       unsigned int nwords = DIV_ROUND_UP(nblocks * bits_per_block,
+                                          BITS_PER_LONG);
+
+       nand->bbt.cache = kzalloc(nwords, GFP_KERNEL);

Quoted 8 lines

drivers/mtd/nand/bbt.c line 38

Quoted 6 lines

drivers/mtd/nand/bbt.c new file +6 -0

+       if (!nand->bbt.cache)
+               return -ENOMEM;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(nanddev_bbt_init);  

Can't we skip __UBOOT__ and EXPORT_SYMBOL_GPL ?

Do you mean that you want me to delete all the #ifndef __UBOOT__/#endif sections?

They are present only to ease the patch backporting process from Linux. It was very useful during the development, I don't have a strong opinion on whether we should keep them or not, yet.

From: Boris Brezillon <[email protected]>

Some controllers are exposing high-level interfaces to access various kind of SPI memories. Unfortunately they do not fit in the current spi_controller model and usually have drivers placed in drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI memories in general.

This is an attempt at defining a SPI memory interface which works for all kinds of SPI memories (NORs, NANDs, SRAMs).

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/spi/Kconfig   |   7 +
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-mem.c | 500 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spi-mem.h     | 258 ++++++++++++++++++++++++++
 include/spi.h         |  11 ++
 5 files changed, 777 insertions(+)
 create mode 100644 drivers/spi/spi-mem.c
 create mode 100644 include/spi-mem.h

drivers/spi/Kconfig +7 -0

index 235a8c7d73..0ee371b2d9 100644
@@ -15,6 +15,13 @@ config DM_SPI
 
 if DM_SPI
 
+config SPI_MEM
+       bool "SPI memory extension"
+       help
+         Enable this option if you want to enable the SPI memory extension.
+         This extension is meant to simplify interaction with SPI memories
+         by providing an high-level interface to send memory-like commands.
+
 config ALTERA_SPI
        bool "Altera SPI driver"
        help

drivers/spi/Makefile +1 -0

index 4b6000fd9a..982529a0e6 100644
@@ -10,6 +10,7 @@ ifdef CONFIG_DM_SPI
 obj-y += spi-uclass.o
 obj-$(CONFIG_SANDBOX) += spi-emul-uclass.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi.o
+obj-$(CONFIG_SPI_MEM) += spi-mem.o
 else
 obj-y += spi.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi_legacy.o

drivers/spi/spi-mem.c new file +8 -0

new file mode 100644
index 0000000000..1aabe56819
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Exceet Electronics GmbH
+ * Copyright (C) 2018 Bootlin
+ *
+ * Author: Boris Brezillon <[email protected]>
+ */
+

Quoted 74 lines

From: Boris Brezillon <[email protected]>

Some controllers are exposing high-level interfaces to access various kind of SPI memories. Unfortunately they do not fit in the current spi_controller model and usually have drivers placed in drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI memories in general.

This is an attempt at defining a SPI memory interface which works for all kinds of SPI memories (NORs, NANDs, SRAMs).

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/spi/Kconfig   |   7 +
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-mem.c | 500 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spi-mem.h     | 258 ++++++++++++++++++++++++++
 include/spi.h         |  11 ++
 5 files changed, 777 insertions(+)
 create mode 100644 drivers/spi/spi-mem.c
 create mode 100644 include/spi-mem.h

drivers/spi/Kconfig +7 -0

index 235a8c7d73..0ee371b2d9 100644
@@ -15,6 +15,13 @@ config DM_SPI
 
 if DM_SPI
 
+config SPI_MEM
+       bool "SPI memory extension"
+       help
+         Enable this option if you want to enable the SPI memory extension.
+         This extension is meant to simplify interaction with SPI memories
+         by providing an high-level interface to send memory-like commands.
+
 config ALTERA_SPI
        bool "Altera SPI driver"
        help

drivers/spi/Makefile +1 -0

index 4b6000fd9a..982529a0e6 100644
@@ -10,6 +10,7 @@ ifdef CONFIG_DM_SPI
 obj-y += spi-uclass.o
 obj-$(CONFIG_SANDBOX) += spi-emul-uclass.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi.o
+obj-$(CONFIG_SPI_MEM) += spi-mem.o
 else
 obj-y += spi.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi_legacy.o

drivers/spi/spi-mem.c new file +15 -0

new file mode 100644
index 0000000000..1aabe56819
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Exceet Electronics GmbH
+ * Copyright (C) 2018 Bootlin
+ *
+ * Author: Boris Brezillon <[email protected]>
+ */
+
+#ifndef __UBOOT__
+#include <linux/dmaengine.h>
+#include <linux/pm_runtime.h>
+#include "internals.h"
+#else
+#include <spi.h>
+#include <spi-mem.h>

drivers/spi/spi-mem.c line 18

Quoted 3 lines

drivers/spi/spi-mem.c new file +3 -0

+#endif
+
+#ifndef __UBOOT__  

I would like remove Linux stuff atleast on this file becuase it's difficult for me to read or review the code and also driver/spi have fully u-boot dm stuff. I know it's easy for Linux sync but for this we can do manual sync what ever need. I'm on something what from Linux.

Hi Jagan,

Jagan Teki <[email protected]> wrote on Fri, 6 Jul 2018 17:02:22 +0530:

Quoted 75 lines · Miquel Raynal

Quoted 74 lines

From: Boris Brezillon <[email protected]>

Some controllers are exposing high-level interfaces to access various kind of SPI memories. Unfortunately they do not fit in the current spi_controller model and usually have drivers placed in drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI memories in general.

This is an attempt at defining a SPI memory interface which works for all kinds of SPI memories (NORs, NANDs, SRAMs).

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/spi/Kconfig   |   7 +
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-mem.c | 500 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spi-mem.h     | 258 ++++++++++++++++++++++++++
 include/spi.h         |  11 ++
 5 files changed, 777 insertions(+)
 create mode 100644 drivers/spi/spi-mem.c
 create mode 100644 include/spi-mem.h

drivers/spi/Kconfig +7 -0

index 235a8c7d73..0ee371b2d9 100644
@@ -15,6 +15,13 @@ config DM_SPI
 
 if DM_SPI
 
+config SPI_MEM
+       bool "SPI memory extension"
+       help
+         Enable this option if you want to enable the SPI memory extension.
+         This extension is meant to simplify interaction with SPI memories
+         by providing an high-level interface to send memory-like commands.
+
 config ALTERA_SPI
        bool "Altera SPI driver"
        help

drivers/spi/Makefile +1 -0

index 4b6000fd9a..982529a0e6 100644
@@ -10,6 +10,7 @@ ifdef CONFIG_DM_SPI
 obj-y += spi-uclass.o
 obj-$(CONFIG_SANDBOX) += spi-emul-uclass.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi.o
+obj-$(CONFIG_SPI_MEM) += spi-mem.o
 else
 obj-y += spi.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi_legacy.o

drivers/spi/spi-mem.c new file +15 -0

new file mode 100644
index 0000000000..1aabe56819
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Exceet Electronics GmbH
+ * Copyright (C) 2018 Bootlin
+ *
+ * Author: Boris Brezillon <[email protected]>
+ */
+
+#ifndef __UBOOT__
+#include <linux/dmaengine.h>
+#include <linux/pm_runtime.h>
+#include "internals.h"
+#else
+#include <spi.h>
+#include <spi-mem.h>

Quoted 8 lines

drivers/spi/spi-mem.c line 18

Quoted 3 lines

drivers/spi/spi-mem.c new file +3 -0

+#endif
+
+#ifndef __UBOOT__

I would like remove Linux stuff atleast on this file becuase it's difficult for me to read or review the code and also driver/spi have fully u-boot dm stuff. I know it's easy for Linux sync but for this we can do manual sync what ever need. I'm on something what from Linux.

I'm not sure this is a wise idea.

And I don't understand how "driver/spi have fully u-boot dm stuff" is related in any manner to this request.

Quoted 91 lines · Miquel Raynal

Hi Jagan,

Jagan Teki <[email protected]> wrote on Fri, 6 Jul 2018 17:02:22 +0530:

Quoted 75 lines · Miquel Raynal

Quoted 74 lines

From: Boris Brezillon <[email protected]>

Some controllers are exposing high-level interfaces to access various kind of SPI memories. Unfortunately they do not fit in the current spi_controller model and usually have drivers placed in drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI memories in general.

This is an attempt at defining a SPI memory interface which works for all kinds of SPI memories (NORs, NANDs, SRAMs).

Signed-off-by Boris Brezillon <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/spi/Kconfig   |   7 +
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-mem.c | 500 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spi-mem.h     | 258 ++++++++++++++++++++++++++
 include/spi.h         |  11 ++
 5 files changed, 777 insertions(+)
 create mode 100644 drivers/spi/spi-mem.c
 create mode 100644 include/spi-mem.h

drivers/spi/Kconfig +7 -0

index 235a8c7d73..0ee371b2d9 100644
@@ -15,6 +15,13 @@ config DM_SPI
 
 if DM_SPI
 
+config SPI_MEM
+       bool "SPI memory extension"
+       help
+         Enable this option if you want to enable the SPI memory extension.
+         This extension is meant to simplify interaction with SPI memories
+         by providing an high-level interface to send memory-like commands.
+
 config ALTERA_SPI
        bool "Altera SPI driver"
        help

drivers/spi/Makefile +1 -0

index 4b6000fd9a..982529a0e6 100644
@@ -10,6 +10,7 @@ ifdef CONFIG_DM_SPI
 obj-y += spi-uclass.o
 obj-$(CONFIG_SANDBOX) += spi-emul-uclass.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi.o
+obj-$(CONFIG_SPI_MEM) += spi-mem.o
 else
 obj-y += spi.o
 obj-$(CONFIG_SOFT_SPI) += soft_spi_legacy.o

drivers/spi/spi-mem.c new file +15 -0

new file mode 100644
index 0000000000..1aabe56819
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Exceet Electronics GmbH
+ * Copyright (C) 2018 Bootlin
+ *
+ * Author: Boris Brezillon <[email protected]>
+ */
+
+#ifndef __UBOOT__
+#include <linux/dmaengine.h>
+#include <linux/pm_runtime.h>
+#include "internals.h"
+#else
+#include <spi.h>
+#include <spi-mem.h>

Quoted 8 lines

drivers/spi/spi-mem.c line 18

Quoted 3 lines

drivers/spi/spi-mem.c new file +3 -0

+#endif
+
+#ifndef __UBOOT__  

I would like remove Linux stuff atleast on this file becuase it's difficult for me to read or review the code and also driver/spi have fully u-boot dm stuff. I know it's easy for Linux sync but for this we can do manual sync what ever need. I'm on something what from Linux.

I'm not sure this is a wise idea.

Quoted 2 lines

And I don't understand how "driver/spi have fully u-boot dm stuff" is related in any manner to this request.

The code in driver/spi is more or less u-boot code it doesn't have Linux sync. ie reason I want to maintain the similar integrity here, but on the other-side spi-mem depend more on Linux like MTD. OK let's move as-it-is like what you added, may be we can simulate if require in future need.

Add support for the MX35LF2GE4AB chip, which is similar to its cousin MX35LF1GE4AB, with two planes instead of one.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/mtd/nand/spi/macronix.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

drivers/mtd/nand/spi/macronix.c +5 -5

index dd351dcb6c..d761b99d26 100644
@@ -27,13 +27,13 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
 		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
 		SPINAND_PROG_LOAD(false, 0, NULL, 0));
 
-static int mx35lf1ge4ab_ooblayout_ecc(struct mtd_info *mtd, int section,
+static int mx35lfxge4ab_ooblayout_ecc(struct mtd_info *mtd, int section,
 				      struct mtd_oob_region *region)
 {
 	return -ERANGE;
 }
 
-static int mx35lf1ge4ab_ooblayout_free(struct mtd_info *mtd, int section,
+static int mx35lfxge4ab_ooblayout_free(struct mtd_info *mtd, int section,
 				       struct mtd_oob_region *region)
 {
 	if (section)

@@ -45,9 +45,9 @@ static int mx35lf1ge4ab_ooblayout_free(struct mtd_info *mtd, int section,
 	return 0;
 }
 
-static const struct mtd_ooblayout_ops mx35lf1ge4ab_ooblayout = {
-	.ecc = mx35lf1ge4ab_ooblayout_ecc,
-	.free = mx35lf1ge4ab_ooblayout_free,
+static const struct mtd_ooblayout_ops mx35lfxge4ab_ooblayout = {
+	.ecc = mx35lfxge4ab_ooblayout_ecc,
+	.free = mx35lfxge4ab_ooblayout_free,
 };
 
 static int mx35lf1ge4ab_get_eccsr(struct spinand_device *spinand, u8 *eccsr)

drivers/mtd/nand/spi/macronix.c +6 -0

+		     NAND_ECCREQ(4, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     SPINAND_HAS_QE_BIT,
+		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)),
 };
 
 static int macronix_spinand_detect(struct spinand_device *spinand)

Quoted 40 lines · On Wed, 6 Jun 2018 17:30:35 +0200 Miquel Raynal <[email protected]>…

Add support for the MX35LF2GE4AB chip, which is similar to its cousin MX35LF1GE4AB, with two planes instead of one.

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/mtd/nand/spi/macronix.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

drivers/mtd/nand/spi/macronix.c +5 -5

index dd351dcb6c..d761b99d26 100644
@@ -27,13 +27,13 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
 		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
 		SPINAND_PROG_LOAD(false, 0, NULL, 0));
 
-static int mx35lf1ge4ab_ooblayout_ecc(struct mtd_info *mtd, int section,
+static int mx35lfxge4ab_ooblayout_ecc(struct mtd_info *mtd, int section,
 				      struct mtd_oob_region *region)
 {
 	return -ERANGE;
 }
 
-static int mx35lf1ge4ab_ooblayout_free(struct mtd_info *mtd, int section,
+static int mx35lfxge4ab_ooblayout_free(struct mtd_info *mtd, int section,
 				       struct mtd_oob_region *region)
 {
 	if (section)

@@ -45,9 +45,9 @@ static int mx35lf1ge4ab_ooblayout_free(struct mtd_info *mtd, int section,
 	return 0;
 }
 
-static const struct mtd_ooblayout_ops mx35lf1ge4ab_ooblayout = {
-	.ecc = mx35lf1ge4ab_ooblayout_ecc,
-	.free = mx35lf1ge4ab_ooblayout_free,
+static const struct mtd_ooblayout_ops mx35lfxge4ab_ooblayout = {
+	.ecc = mx35lfxge4ab_ooblayout_ecc,
+	.free = mx35lfxge4ab_ooblayout_free,
 };
 

drivers/mtd/nand/spi/macronix.c static const struct spinand_info macronix_spinand_table[] = { · line 108

Quoted 10 lines

drivers/mtd/nand/spi/macronix.c +3 -1

 static int mx35lf1ge4ab_get_eccsr(struct spinand_device *spinand, u8 *eccsr)

@@ -102,8 +102,16 @@ static const struct spinand_info macronix_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     SPINAND_HAS_QE_BIT,
-		     SPINAND_ECCINFO(&mx35lf1ge4ab_ooblayout,
+		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
 				     mx35lf1ge4ab_ecc_get_status)),
+	SPINAND_INFO("MX35LF2GE4AB", 0x22,
+		     NAND_MEMORG(1, 2048, 64, 64, 1024, 2, 1, 1),  

I think it should be 2048 here ^.

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

Signed-off-by Miquel Raynal <[email protected]>

---
 cmd/Kconfig          |   5 +
 cmd/Makefile         |   1 +
 cmd/mtd.c            | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/Makefile |   2 +-
 4 files changed, 287 insertions(+), 1 deletion(-)
 create mode 100644 cmd/mtd.c

cmd/Kconfig +5 -0

index 136836d146..6e9b629e1c 100644
@@ -797,6 +797,11 @@ config CMD_MMC
 	help
 	  MMC memory mapped support.
 
+config CMD_MTD
+	bool "mtd"
+	help
+	  MTD commands support.
+
 config CMD_NAND
 	bool "nand"
 	default y if NAND_SUNXI

cmd/Makefile +1 -0

index 9a358e4801..e42db12e1d 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_CMD_MISC) += misc.o
 obj-$(CONFIG_CMD_MMC) += mmc.o
 obj-$(CONFIG_CMD_MMC_SPI) += mmc_spi.o
 obj-$(CONFIG_MP) += mp.o
+obj-$(CONFIG_CMD_MTD) += mtd.o
 obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
 obj-$(CONFIG_CMD_NAND) += nand.o
 obj-$(CONFIG_CMD_NET) += net.o

cmd/mtd.c new file +51 -0

new file mode 100644
index 0000000000..fe48378bd0
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier:  GPL-2.0+
+/*
+ * mtd.c
+ *
+ * Generic command to handle basic operations on any memory device.
+ *
+ * Copyright: Bootlin, 2018
+ * Author: Miquèl Raynal <[email protected]>
+ */
+
+#include <common.h>
+#include <linux/mtd/mtd.h>
+#include <command.h>
+#include <console.h>
+#include <malloc.h>
+#include <mtd.h>
+#include <mapmem.h>
+#include <dm/device.h>
+#include <dm/uclass-internal.h>
+
+static void mtd_dump_buf(u8 *buf, uint len)
+{
+	int i, j;
+
+	for (i = 0; i < len; ) {
+		printf("0x%08x:\t", i);
+		for (j = 0; j < 8; j++)
+			printf("%02x ", buf[i + j]);
+		printf(" ");
+		i += 8;
+		for (j = 0; j < 8; j++)
+			printf("%02x ", buf[i + j]);
+		printf("\n");
+		i += 8;
+	}
+}
+
+static void mtd_show_device(struct mtd_info *mtd)
+{
+	printf("* %s", mtd->name);
+	if (mtd->dev)
+		printf(" [device: %s] [parent: %s] [driver: %s]",
+		       mtd->dev->name, mtd->dev->parent->name,
+		       mtd->dev->driver->name);
+
+	printf("\n");
+}
+
+static int do_mtd_list(void)
+{
+	struct mtd_info *mtd;

cmd/mtd.c new file +4 -0

+
+	mtd_for_each_device(mtd) {
+		mtd_show_device(mtd);
+		idx++;

cmd/mtd.c new file +20 -0

+	struct mtd_oob_ops ops = {
+		.mode = raw ? MTD_OPS_RAW : 0,
+		.len = len,
+		.ooblen = woob ? mtd->oobsize : 0,
+		.datbuf = buf,
+		.oobbuf = woob ? &buf[mtd->writesize] : NULL,
+	};
+	int ret;
+
+	if (!buf)
+		return -ENOMEM;
+
+	memset(buf, 0xFF, buf_len);
+
+	if (read) {
+		ret = mtd_read_oob(mtd, from, &ops);
+	} else {
+		memcpy(buf, waddr, ops.len + ops.ooblen);
+		ret = mtd_write_oob(mtd, from, &ops);
+	}

cmd/mtd.c new file +36 -0

+	}
+
+	kfree(buf);
+
+	return 0;
+}
+
+static int do_mtd_erase(struct mtd_info *mtd, bool scrub, u64 from, u64 len)
+{
+	struct erase_info erase_infos = {
+		.mtd = mtd,
+		.addr = from,
+		.len = len,
+		.scrub = scrub,
+	};
+
+	return mtd_erase(mtd, &erase_infos);
+}
+
+static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct mtd_info *mtd;
+	struct udevice *dev;
+	const char *cmd;
+	char *part;
+	int ret;
+
+	/* All MTD commands need at least two arguments */
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	/* Parse the command name and its optional suffixes */
+	cmd = argv[1];
+
+	/* List the MTD devices if that is what the user wants */
+	if (strcmp(cmd, "list") == 0)

cmd/mtd.c new file +6 -0

+	ret = uclass_find_device_by_name(UCLASS_MTD, part, &dev);
+	if (!ret && dev) {
+		mtd_probe(dev);
+		mtd = (struct mtd_info *)dev_get_uclass_priv(dev);
+		if (!mtd) {
+			printf("Could not retrieve MTD data\n");

cmd/mtd.c new file +28 -0

+
+	argc -= 3;
+	argv += 3;
+
+	/* Do the parsing */
+	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "write", 5)) {
+		bool read, raw, woob;
+		uint *waddr = NULL;
+		u64 off, len;
+
+		read = !strncmp(cmd, "read", 4);
+		raw = strstr(cmd, ".raw");
+		woob = strstr(cmd, ".oob");
+
+		if (!read) {
+			if (argc < 1)
+				return CMD_RET_USAGE;
+
+			waddr = map_sysmem(simple_strtoul(argv[0], NULL, 10),
+					   0);
+			argc--;
+			argv++;
+		}
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->writesize + (woob ? mtd->oobsize : 0);
+

cmd/mtd.c new file +46 -0

+			printf("Access location beyond the end of the chip\n");
+			return -EINVAL;
+		}
+
+		printf("%s (from %p) %lldB at 0x%08llx [%s %s]\n",
+		       read ? "read" : "write", read ? 0 : waddr, len, off,
+		       raw ? "raw" : "", woob ? "oob" : "");
+
+		ret = do_mtd_read_write(mtd, read, waddr, raw, woob, off, len);
+
+		if (!read)
+			unmap_sysmem(waddr);
+
+	} else if (!strcmp(cmd, "erase") || !strcmp(cmd, "scrub")) {
+		bool scrub = !strcmp(cmd, "scrub");
+		bool full_erase = !strncmp(&cmd[5], ".chip", 4);
+		u64 off, len;
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->erasesize;
+		if (full_erase) {
+			off = 0;
+			len = mtd->size;
+		}
+
+		if ((u32)off % mtd->erasesize) {
+			printf("Section not erase-block-aligned (0x%x)\n",
+			       mtd->erasesize);
+			return -EINVAL;
+		}
+
+		if ((u32)len % mtd->erasesize) {
+			printf("Size not aligned with an erase block (%dB)\n",
+			       mtd->erasesize);
+			return -EINVAL;
+		}
+
+		if ((off + len) >= mtd->size) {
+			printf("Cannot read beyond end of chip\n");
+			return -EINVAL;
+		}
+
+		ret = do_mtd_erase(mtd, scrub, off, len);
+	} else {
+		return CMD_RET_USAGE;

Hi Miquel,

Quoted 4 lines · On Wed, 6 Jun 2018 17:30:37 +0200 Miquel Raynal <[email protected]>…

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

Just a few comments below, but overall, I'm really happy with this new set of commands and the fact that we'll soon be able to replace custom MTD accessors (nand, onenand, sf, cp.b+erase, ...) by these ones.

Quoted 103 lines

Signed-off-by Miquel Raynal <[email protected]>

---
 cmd/Kconfig          |   5 +
 cmd/Makefile         |   1 +
 cmd/mtd.c            | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/Makefile |   2 +-
 4 files changed, 287 insertions(+), 1 deletion(-)
 create mode 100644 cmd/mtd.c

cmd/Kconfig +5 -0

index 136836d146..6e9b629e1c 100644
@@ -797,6 +797,11 @@ config CMD_MMC
 	help
 	  MMC memory mapped support.
 
+config CMD_MTD
+	bool "mtd"
+	help
+	  MTD commands support.
+
 config CMD_NAND
 	bool "nand"
 	default y if NAND_SUNXI

cmd/Makefile +1 -0

index 9a358e4801..e42db12e1d 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_CMD_MISC) += misc.o
 obj-$(CONFIG_CMD_MMC) += mmc.o
 obj-$(CONFIG_CMD_MMC_SPI) += mmc_spi.o
 obj-$(CONFIG_MP) += mp.o
+obj-$(CONFIG_CMD_MTD) += mtd.o
 obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
 obj-$(CONFIG_CMD_NAND) += nand.o
 obj-$(CONFIG_CMD_NET) += net.o

cmd/mtd.c new file +59 -0

new file mode 100644
index 0000000000..fe48378bd0
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier:  GPL-2.0+
+/*
+ * mtd.c
+ *
+ * Generic command to handle basic operations on any memory device.
+ *
+ * Copyright: Bootlin, 2018
+ * Author: Miquèl Raynal <[email protected]>
+ */
+
+#include <common.h>
+#include <linux/mtd/mtd.h>
+#include <command.h>
+#include <console.h>
+#include <malloc.h>
+#include <mtd.h>
+#include <mapmem.h>
+#include <dm/device.h>
+#include <dm/uclass-internal.h>
+
+static void mtd_dump_buf(u8 *buf, uint len)
+{
+	int i, j;
+
+	for (i = 0; i < len; ) {
+		printf("0x%08x:\t", i);
+		for (j = 0; j < 8; j++)
+			printf("%02x ", buf[i + j]);
+		printf(" ");
+		i += 8;
+		for (j = 0; j < 8; j++)
+			printf("%02x ", buf[i + j]);
+		printf("\n");
+		i += 8;
+	}
+}
+
+static void mtd_show_device(struct mtd_info *mtd)
+{
+	printf("* %s", mtd->name);
+	if (mtd->dev)
+		printf(" [device: %s] [parent: %s] [driver: %s]",
+		       mtd->dev->name, mtd->dev->parent->name,
+		       mtd->dev->driver->name);
+
+	printf("\n");
+}
+
+static int do_mtd_list(void)
+{
+	struct mtd_info *mtd;
+	struct udevice *dev;
+	int dm_idx = 0, idx = 0;
+
+	/* Ensure all devices compliants with U-Boot driver model are probed */
+	while (!uclass_find_device(UCLASS_MTD, dm_idx, &dev) && dev) {
+		mtd_probe(dev);
+		dm_idx++;
+	}

cmd/mtd.c line 61

Quoted 2 lines

cmd/mtd.c new file +2 -0

+
+	printf("MTD devices list (%d DM compliant):\n", dm_idx);  

Do we really want to say how many of them are exported by DM compliant drivers? I mean, the user doesn't care about that. If you want to force people to convert their drivers, we should probably complain at MTD device registration time when the mtd_info struct is not backed by an udevice.

Quoted 5 lines

cmd/mtd.c new file +4 -0

+
+	mtd_for_each_device(mtd) {
+		mtd_show_device(mtd);
+		idx++;

cmd/mtd.c line 75

Quoted 10 lines

cmd/mtd.c new file +10 -0

+	}
+
+	if (!idx)
+		printf("No MTD device found\n");
+
+	return 0;
+}
+
+static int do_mtd_read_write(struct mtd_info *mtd, bool read, uint *waddr,
+			     bool raw, bool woob, u64 from, u64 len)  

s/do_mtd_read_write/do_mtd_io/ ?

And why not passing an mtd_oob_ops
object directly? That would reduce the number of parameters you pass to this function.

cmd/mtd.c line 79

Quoted 5 lines

cmd/mtd.c new file +4 -0

+{
+	u32 buf_len = woob ? mtd->writesize + mtd->oobsize :
+			     ROUND(len, mtd->writesize);
+	u8 *buf = malloc(buf_len);  

It's probably worth a comment explaining why you allocate a bounce buffer here (i.e. to make sure len not aligned on a page size are padded with 0xff).

Maybe a simpler solution would be to simply refuse such unaligned accesses.

Quoted 26 lines

cmd/mtd.c new file +25 -0

+	struct mtd_oob_ops ops = {
+		.mode = raw ? MTD_OPS_RAW : 0,
+		.len = len,
+		.ooblen = woob ? mtd->oobsize : 0,
+		.datbuf = buf,
+		.oobbuf = woob ? &buf[mtd->writesize] : NULL,
+	};
+	int ret;
+
+	if (!buf)
+		return -ENOMEM;
+
+	memset(buf, 0xFF, buf_len);
+
+	if (read) {
+		ret = mtd_read_oob(mtd, from, &ops);
+	} else {
+		memcpy(buf, waddr, ops.len + ops.ooblen);
+		ret = mtd_write_oob(mtd, from, &ops);
+	}
+
+	if (ret) {
+		printf("Could not handle %lldB from 0x%08llx on %s, ret %d\n",
+		       len, from, mtd->name, ret);
+		return ret;

cmd/mtd.c line 109

Quoted 5 lines

cmd/mtd.c new file +5 -0

+	}
+
+	if (read) {
+		printf("Dump %lld data bytes from 0x%08llx:\n", len, from);
+		mtd_dump_buf(buf, len);  

Read and dump are 2 different things: one might want to read an MTD device and store the result in RAM without dumping it on the console.

cmd/mtd.c line 115

Quoted 7 lines

cmd/mtd.c new file +6 -0

+
+		if (woob) {
+			printf("\nDump %d OOB bytes from 0x%08llx:\n",
+			       mtd->oobsize, from);
+			mtd_dump_buf(&buf[len], mtd->oobsize);
+		}  

Looks like you're never copying the data back to waddr.

Quoted 42 lines

cmd/mtd.c new file +41 -0

+	}
+
+	kfree(buf);
+
+	return 0;
+}
+
+static int do_mtd_erase(struct mtd_info *mtd, bool scrub, u64 from, u64 len)
+{
+	struct erase_info erase_infos = {
+		.mtd = mtd,
+		.addr = from,
+		.len = len,
+		.scrub = scrub,
+	};
+
+	return mtd_erase(mtd, &erase_infos);
+}
+
+static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct mtd_info *mtd;
+	struct udevice *dev;
+	const char *cmd;
+	char *part;
+	int ret;
+
+	/* All MTD commands need at least two arguments */
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	/* Parse the command name and its optional suffixes */
+	cmd = argv[1];
+
+	/* List the MTD devices if that is what the user wants */
+	if (strcmp(cmd, "list") == 0)
+		return do_mtd_list();
+
+	/*
+	 * The remaining commands require also at least a device ID.
+	 * Check the selected device is valid. Ensure it is probed.

cmd/mtd.c line 161

Quoted 5 lines

cmd/mtd.c new file +5 -0

+	 */
+	if (argc < 3)
+		return CMD_RET_USAGE;
+
+	part = argv[2];  

Why part. The MTD object can be a partition or the device itself. How about renaming it mtdname?

Quoted 15 lines

cmd/mtd.c new file +14 -0

+	ret = uclass_find_device_by_name(UCLASS_MTD, part, &dev);
+	if (!ret && dev) {
+		mtd_probe(dev);
+		mtd = (struct mtd_info *)dev_get_uclass_priv(dev);
+		if (!mtd) {
+			printf("Could not retrieve MTD data\n");
+			return -ENODEV;
+		}
+	} else {
+		mtd = get_mtd_device_nm(part);
+		if (IS_ERR_OR_NULL(mtd)) {
+			printf("MTD device %s not found, ret %ld\n", part,
+			       PTR_ERR(mtd));
+			return 1;

cmd/mtd.c line 177

Quoted 2 lines

cmd/mtd.c new file +2 -0

+		}
+	}  

Hm, I'd do it the other way around: first call get_mtd_device_nm() and if you don't find the device trigger the probe of all UCLASS_MTD devs, and then search again with get_mtd_device_nm(). Note that mtd->dev->name and mtd->name are 2 different things, and they won't match most of the time.

Quoted 34 lines

cmd/mtd.c new file +33 -0

+
+	argc -= 3;
+	argv += 3;
+
+	/* Do the parsing */
+	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "write", 5)) {
+		bool read, raw, woob;
+		uint *waddr = NULL;
+		u64 off, len;
+
+		read = !strncmp(cmd, "read", 4);
+		raw = strstr(cmd, ".raw");
+		woob = strstr(cmd, ".oob");
+
+		if (!read) {
+			if (argc < 1)
+				return CMD_RET_USAGE;
+
+			waddr = map_sysmem(simple_strtoul(argv[0], NULL, 10),
+					   0);
+			argc--;
+			argv++;
+		}
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->writesize + (woob ? mtd->oobsize : 0);
+
+		if ((u32)off % mtd->writesize) {
+			printf("Section not page-aligned (0x%x)\n",
+			       mtd->writesize);
+			return -EINVAL;
+		}

cmd/mtd.c line 215

Quoted 5 lines

cmd/mtd.c new file +5 -0

+
+		if (woob && (len != (mtd->writesize + mtd->oobsize))) {
+			printf("OOB operations are limited to single pages\n");
+			return -EINVAL;
+		}  

Is this a uboot limitation? I don't think you have such a limitation in Linux.

cmd/mtd.c line 217

Quoted 3 lines

cmd/mtd.c new file +2 -0

+
+		if ((off + len) >= mtd->size) {  

That doesn't work when reading the last page of the MTD device with woob = true. See how Linux handle that here [1]. BTW, why don't you let mtdcore.c do these checks for you (that's also true for unaligned accesses)?

Quoted 47 lines

cmd/mtd.c new file +46 -0

+			printf("Access location beyond the end of the chip\n");
+			return -EINVAL;
+		}
+
+		printf("%s (from %p) %lldB at 0x%08llx [%s %s]\n",
+		       read ? "read" : "write", read ? 0 : waddr, len, off,
+		       raw ? "raw" : "", woob ? "oob" : "");
+
+		ret = do_mtd_read_write(mtd, read, waddr, raw, woob, off, len);
+
+		if (!read)
+			unmap_sysmem(waddr);
+
+	} else if (!strcmp(cmd, "erase") || !strcmp(cmd, "scrub")) {
+		bool scrub = !strcmp(cmd, "scrub");
+		bool full_erase = !strncmp(&cmd[5], ".chip", 4);
+		u64 off, len;
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->erasesize;
+		if (full_erase) {
+			off = 0;
+			len = mtd->size;
+		}
+
+		if ((u32)off % mtd->erasesize) {
+			printf("Section not erase-block-aligned (0x%x)\n",
+			       mtd->erasesize);
+			return -EINVAL;
+		}
+
+		if ((u32)len % mtd->erasesize) {
+			printf("Size not aligned with an erase block (%dB)\n",
+			       mtd->erasesize);
+			return -EINVAL;
+		}
+
+		if ((off + len) >= mtd->size) {
+			printf("Cannot read beyond end of chip\n");
+			return -EINVAL;
+		}
+
+		ret = do_mtd_erase(mtd, scrub, off, len);
+	} else {
+		return CMD_RET_USAGE;

cmd/mtd.c line 273

Quoted 10 lines

cmd/mtd.c new file +10 -0

+	}
+
+	return ret;
+}
+
+static char mtd_help_text[] =
+#ifdef CONFIG_SYS_LONGHELP
+	"- generic operations on memory technology devices\n\n"
+	"mtd list\n"
+	"mtd read[.raw][.oob] <name> [<off> [<size>]]\n"  

I guess this one should be

	"mtd read[.raw][.oob] <name> <addr> [<off> [<size>]]\n"

and then, you should have

	"mtd dump[.raw][.oob] <name> [<off> [<size>]]\n"

cmd/mtd.c line 276

Quoted 3 lines

cmd/mtd.c new file +3 -0

+	"mtd write[.raw][.oob] <name> <addr> [<off> [<size>]]\n"
+	"mtd erase[.chip] <name> [<off> [<size>]]\n"
+	"mtd scrub[.chip] <name> [<off> [<size>]]\n"  

Hm, maybe it's time to simplify that. mtd.scrub is just an option of mtd erase, so maybe we should just have:

mtd erase[.force] or erase[.dontskipbad]

Also, [.chip] can be extracted from the number of parameters. If you just have <name> passed, that means the callers wants to erase the whole chip.

Regards,

Boris

cmd/mtd.c line 280

Quoted 4 lines

cmd/mtd.c new file +4 -0

+#endif
+	"";
+
+U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);  

[1]https://elixir.bootlin.com/linux/latest/source/drivers/mtd/mtdcore.c#L1117

Quoted 33 lines

Quoted 33 lines

cmd/mtd.c new file +33 -0

+
+	argc -= 3;
+	argv += 3;
+
+	/* Do the parsing */
+	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "write", 5)) {
+		bool read, raw, woob;
+		uint *waddr = NULL;
+		u64 off, len;
+
+		read = !strncmp(cmd, "read", 4);
+		raw = strstr(cmd, ".raw");
+		woob = strstr(cmd, ".oob");
+
+		if (!read) {
+			if (argc < 1)
+				return CMD_RET_USAGE;
+
+			waddr = map_sysmem(simple_strtoul(argv[0], NULL, 10),
+					   0);
+			argc--;
+			argv++;
+		}
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->writesize + (woob ? mtd->oobsize : 0);
+
+		if ((u32)off % mtd->writesize) {
+			printf("Section not page-aligned (0x%x)\n",
+			       mtd->writesize);
+			return -EINVAL;
+		}

Quoted 8 lines

cmd/mtd.c line 215

Quoted 5 lines

cmd/mtd.c new file +5 -0

+
+		if (woob && (len != (mtd->writesize + mtd->oobsize))) {
+			printf("OOB operations are limited to single pages\n");
+			return -EINVAL;
+		}    

Is this a uboot limitation? I don't think you have such a limitation in Linux.

Kind of, only one single page write with OOB at a time is possible says a comment on mtd_oob_ops in mtd.h in Linux. Reads are actually not limited. But I really prefer to keep this limitation that simplifies _a lot_ the logic and is not really useful to a u-boot user I suppose.

Quoted 9 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+		if ((off + len) >= mtd->size) {    

That doesn't work when reading the last page of the MTD device with woob = true. See how Linux handle that here [1]. BTW, why don't you let mtdcore.c do these checks for you (that's also true for unaligned accesses)?

Because the relevant patch (and its fix :) ) has not been backported yet.

And now I understand your voice in my ears "do it".

Quoted 42 lines · On Wed, 11 Jul 2018 15:51:39 +0200 Miquel Raynal <[email protected]>…

Quoted 41 lines

Quoted 28 lines

cmd/mtd.c new file +28 -0

+
+	argc -= 3;
+	argv += 3;
+
+	/* Do the parsing */
+	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "write", 5)) {
+		bool read, raw, woob;
+		uint *waddr = NULL;
+		u64 off, len;
+
+		read = !strncmp(cmd, "read", 4);
+		raw = strstr(cmd, ".raw");
+		woob = strstr(cmd, ".oob");
+
+		if (!read) {
+			if (argc < 1)
+				return CMD_RET_USAGE;
+
+			waddr = map_sysmem(simple_strtoul(argv[0], NULL, 10),
+					   0);
+			argc--;
+			argv++;
+		}
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) :
+				 mtd->writesize + (woob ? mtd->oobsize : 0);
+

cmd/mtd.c line 215

Quoted 10 lines

cmd/mtd.c new file +10 -0

+		if ((u32)off % mtd->writesize) {
+			printf("Section not page-aligned (0x%x)\n",
+			       mtd->writesize);
+			return -EINVAL;
+		}
+
+		if (woob && (len != (mtd->writesize + mtd->oobsize))) {
+			printf("OOB operations are limited to single pages\n");
+			return -EINVAL;
+		}      

Is this a uboot limitation? I don't think you have such a limitation in Linux.

Quoted 4 lines

Kind of, only one single page write with OOB at a time is possible says a comment on mtd_oob_ops in mtd.h in Linux. Reads are actually not limited. But I really prefer to keep this limitation that simplifies _a lot_ the logic and is not really useful to a u-boot user I suppose.

It is useful when you write things in raw mode and the image you write contains OOB (which in turn contains pre-generated ECC bytes). I know we do such things on sunxi when writing the SPL.

Quoted 11 lines

Quoted 10 lines

Review

Quoted 5 lines

(quoted diff) +2 -0

+
+		if ((off + len) >= mtd->size) {      

That doesn't work when reading the last page of the MTD device with woob = true. See how Linux handle that here [1]. BTW, why don't you let mtdcore.c do these checks for you (that's also true for unaligned accesses)?

Quoted 4 lines

Because the relevant patch (and its fix :) ) has not been backported yet.

And now I understand your voice in my ears "do it".

Hehe. So I guess I'll see this patch in your v2 :-).

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

Quoted 4 lines

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

So are you planning to integrate sf, nand command in future, adding them dm conversion of many stuff. I would like to reommend to go with spinand command. I have been through this and finally added spinor command, which I will going to in soon. once all are converted to dm, it would be easy and meaningfull to have common command.

Jagan Teki <[email protected]> wrote on Fri, 6 Jul 2018 17:08:57 +0530:

Quoted 11 lines · Miquel Raynal

Quoted 4 lines

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

So are you planning to integrate sf, nand command in future, adding them dm conversion of many stuff. I would like to reommend to go with spinand command. I have been through this and finally added spinor command, which I will going to in soon. once all are converted to dm, it would be easy and meaningfull to have common command.

I'm not sure we are in sync about this. The whole point of the previous discussion was to decide whether or not we should make full use of the MTD stack or not. I think it was pretty clear on the fact that people prefer to be close to Linux's architecture on this regard.

MTD being an abstraction of the type of memory, I don't get the point in creating yet another command each time a new type of device is supported. The fact that all the drivers of these devices register to the MTD layer makes it trivial to interact with. So why should we wait?

Hi Miquel,

Quoted 14 lines · Miquel Raynal

Jagan Teki <[email protected]> wrote on Fri, 6 Jul 2018 17:08:57 +0530:

Quoted 6 lines

Quoted 5 lines · Miquel Raynal

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

Quoted 6 lines

So are you planning to integrate sf, nand command in future, adding them dm conversion of many stuff. I would like to reommend to go with spinand command. I have been through this and finally added spinor command, which I will going to in soon. once all are converted to dm, it would be easy and meaningfull to have common command.
I'm not sure we are in sync about this. The whole point of the previous

Quoted 8 lines

discussion was to decide whether or not we should make full use of the MTD stack or not. I think it was pretty clear on the fact that people prefer to be close to Linux's architecture on this regard.

Quoted 1 line

MTD being an abstraction of the type of memory, I don't get the

point in creating yet another command each time a new type of device is supported. The fact that all the drivers of these devices register to the MTD layer makes it trivial to interact with. So why should we wait?

Yes, please don't add new commands for each subsystem / device-type. I like the idea of adding one new command-set (your mtd command) and extending this one to all other device-types over the time.

BTW: I'm testing your SPI NAND patches right now (still struggling with some Gigadevice SPI NAND) and found that the "mtd" command is not really in-line with the usual U-Boot commands. Here some comments:

- Use hex values per default (addresses, sizes and soffset)
- "mtd read" just prints the read values. It makes more sense to read into memory instead (similar to the "mtd write")

I have some patches to address these issues in the queue (still need some massaging), which you can fold into your patchset, once we agree on this.

Hi Stefan,

Stefan Roese <[email protected]> wrote on Fri, 6 Jul 2018 15:21:20 +0200:

Quoted 30 lines

Hi Miquel,

Quoted 14 lines · Miquel Raynal

Jagan Teki <[email protected]> wrote on Fri, 6 Jul 2018 17:08:57 +0530:

Quoted 6 lines

Quoted 5 lines · Miquel Raynal

There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device.

Quoted 6 lines

So are you planning to integrate sf, nand command in future, adding them dm conversion of many stuff. I would like to reommend to go with spinand command. I have been through this and finally added spinor command, which I will going to in soon. once all are converted to dm, it would be easy and meaningfull to have common command.
I'm not sure we are in sync about this. The whole point of the previous

Quoted 8 lines

discussion was to decide whether or not we should make full use of the MTD stack or not. I think it was pretty clear on the fact that people prefer to be close to Linux's architecture on this regard.

Quoted 1 line

MTD being an abstraction of the type of memory, I don't get the

point in creating yet another command each time a new type of device is supported. The fact that all the drivers of these devices register to the MTD layer makes it trivial to interact with. So why should we wait?

Yes, please don't add new commands for each subsystem / device-type. I like the idea of adding one new command-set (your mtd command) and extending this one to all other device-types over the time.

Quoted 8 lines

BTW: I'm testing your SPI NAND patches right now (still struggling with some Gigadevice SPI NAND) and found that the "mtd" command is not really in-line with the usual U-Boot commands. Here some comments:

- Use hex values per default (addresses, sizes and soffset)
- "mtd read" just prints the read values. It makes more sense to read into memory instead (similar to the "mtd write")

I absolutely agree with this! I sent this series to show people what I planned to contribute but this mtd command is still a WIP and there are plenty of things to address.

Quoted 1 line

Quoted 3 lines

I have some patches to address these issues in the queue (still need some massaging), which you can fold into your patchset, once we agree on this.

I am currently working on a new iteration of this series in which the mtd.c file will change quite a bit. I plan to send a new version early next week. I suppose this one will be much more stable to base your fixes/enhancements on.

During the last months, Boris Brezillon shared his work to support serial flashes within Linux. First, he delivered (and merged) a new layer called spi-mem. He also initiated in Linux MTD subsystem the move of all 'raw' NAND related code to a raw/ subdirectory, adding at the same time a NAND core that would be shared with all NAND devices. Then, he contributed a generic SPI-NAND driver, making use of this NAND core, as well as some vendor code to drive a few chips.

+ Boris
+ Suneel (who helped in DM MTD)
+ Siva, Michal (zynq qspi)

Quoted 7 lines · Miquel Raynal

During the last months, Boris Brezillon shared his work to support serial flashes within Linux. First, he delivered (and merged) a new layer called spi-mem. He also initiated in Linux MTD subsystem the move of all 'raw' NAND related code to a raw/ subdirectory, adding at the same time a NAND core that would be shared with all NAND devices. Then, he contributed a generic SPI-NAND driver, making use of this NAND core, as well as some vendor code to drive a few chips.

1) Can you pointed us the Linux changes along with discussion thread about spi-mem, and spi-nand.

2) If my understanding was correct, spi-mem is replacement for spi-nor controller drivers from driver/mtd/spi-nor in Linux.

3) If so is fsl_qspi spi-nor driver moves to drivers/spi area? yes then how does flash changes handled by spi-mem.

4) we have zynq qspi controller which has extensive features like dual flash(stacked and parallel) does spi-mem support these flash specific changes?

5) Better to send spi-mem and spi-nand changes separately, for better reviewing.

6) We have DM MTD layer in ML, better to send the changes on-top [1]

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=20450

Hi Jagan,

Quoted 13 lines · Jagan Teki

+ Boris
+ Suneel (who helped in DM MTD)
+ Siva, Michal (zynq qspi)

Quoted 7 lines · Miquel Raynal

During the last months, Boris Brezillon shared his work to support serial flashes within Linux. First, he delivered (and merged) a new layer called spi-mem. He also initiated in Linux MTD subsystem the move of all 'raw' NAND related code to a raw/ subdirectory, adding at the same time a NAND core that would be shared with all NAND devices. Then, he contributed a generic SPI-NAND driver, making use of this NAND core, as well as some vendor code to drive a few chips.

Quoted 2 lines

1) Can you pointed us the Linux changes along with discussion thread about spi-mem, and spi-nand.

Sure, you can have a look there:

SPI-mem:
http://lists.infradead.org/pipermail/linux-mtd/2018-April/080225.html

SPI-NAND:
http://lists.infradead.org/pipermail/linux-mtd/2018-May/081005.html

Quoted 3 lines

2) If my understanding was correct, spi-mem is replacement for spi-nor controller drivers from driver/mtd/spi-nor in Linux.

It is not 'exactly' a replacement for spi-nor controller drivers but that's the idea. I suggest you to have a look at Boris' blog post about it:
https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/

Quoted 3 lines

3) If so is fsl_qspi spi-nor driver moves to drivers/spi area? yes then how does flash changes handled by spi-mem.

This series does not migrate the SPI-NOR stack to spi-mem. It focuses on SPI-NAND for now. And I don't understand the second sentence.

Quoted 4 lines

4) we have zynq qspi controller which has extensive features like dual flash(stacked and parallel) does spi-mem support these flash specific changes?

This controller is very specific and such support has not yet been added.

Quoted 2 lines

5) Better to send spi-mem and spi-nand changes separately, for better reviewing.

It would mean sending 3 or 4 distinct series, I thought better to give the whole picture but that's your choice.

Quoted 4 lines

6) We have DM MTD layer in ML, better to send the changes on-top [1]

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=20450

This is great to see such effort being made to develop U-Boot driver-model but is there a real need for yet another layer on top of the MTD stack?

I'm looking at mtd-uclass.c for instance, I don't get the need for a mtd_dread() function, all the operations in the mtd_d*() helpers are already handled by mtd/mtdcore.c, no?

And the mtd_ops structure does not bring a lot. Should not we keep it simple and avoid such intermediate layer?

Also, there is the introduction of a spinor command (what about the existing 'sf'?), which is exactly the opposite of what the MTD abstraction would told us to do (thus, the 'mtd' generic command).

Hi Jagan,

Quoted 13 lines · Jagan Teki

+ Boris
+ Suneel (who helped in DM MTD)
+ Siva, Michal (zynq qspi)

Quoted 7 lines · Miquel Raynal

During the last months, Boris Brezillon shared his work to support serial flashes within Linux. First, he delivered (and merged) a new layer called spi-mem. He also initiated in Linux MTD subsystem the move of all 'raw' NAND related code to a raw/ subdirectory, adding at the same time a NAND core that would be shared with all NAND devices. Then, he contributed a generic SPI-NAND driver, making use of this NAND core, as well as some vendor code to drive a few chips.

Quoted 2 lines

1) Can you pointed us the Linux changes along with discussion thread about spi-mem, and spi-nand.

Sure, you can have a look there:

SPI-mem:
http://lists.infradead.org/pipermail/linux-mtd/2018-April/080225.html

SPI-NAND:
http://lists.infradead.org/pipermail/linux-mtd/2018-May/081005.html

Quoted 4 lines

2) If my understanding was correct, spi-mem is replacement for spi-nor controller drivers from driver/mtd/spi-nor in Linux.

It is not 'exactly' a replacement for spi-nor controller drivers but that's the idea. I suggest you to have a look at Boris' blog post about it:
https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/

Quoted 3 lines

3) If so is fsl_qspi spi-nor driver moves to drivers/spi area? yes then how does flash changes handled by spi-mem.

This series does not migrate the SPI-NOR stack to spi-mem. It focuses on SPI-NAND for now. And I don't understand the second sentence.

Quoted 4 lines

4) we have zynq qspi controller which has extensive features like dual flash(stacked and parallel) does spi-mem support these flash specific changes?

This controller is very specific and such support has not yet been added.

Quoted 2 lines

5) Better to send spi-mem and spi-nand changes separately, for better reviewing.

It would mean sending 3 or 4 distinct series, I thought better to give the whole picture but that's your choice.

Quoted 4 lines

6) We have DM MTD layer in ML, better to send the changes on-top [1]

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=20450

This is great to see such effort being made to develop U-Boot driver-model but is there a real need for yet another layer on top of the MTD stack?

I'm looking at mtd-uclass.c for instance, I don't get the need for a mtd_dread() function, all the operations in the mtd_d*() helpers are already handled by mtd/mtdcore.c, no?

And the mtd_ops structure does not bring a lot. Should not we keep it simple and avoid such intermediate layer?

Also, there is the introduction of a spinor command (what about the existing 'sf'?), which is exactly the opposite of what the MTD abstraction would told us to do (thus, the 'mtd' generic command).

I've looked the code on the respective patches, look like most of the code copy from Linux by adding __UBOOT__. I have no issue with Linux copy but we need to structure the code according to U-Boot in the form of driver-model (this series lack with that).

Here are my suggestions, based the MTD work so-far

First we need to design MTD driver-model which can capable to drive one driver from each interface. (not converting all interface drivers at once, that is taking more time and other issues)

Like Linux MTD, U-Boot should have MTD dm for underlying flash devices like nand, parallel nor, spinor etc. So to drive this theory with driver model(with an example of block layer) mtd is common device interaction for most of memory technology flashes like nand, parallel nor, spinor etc, these are treated as interface types wrt u-boot driver model.

Once the respective interface driver bind happen, the uclass driver will pass an 'interface type' to mtd layer to create device for it, for example once spinor ULASS_SPI_NOR driver bind happen, the uclass driver of spinor will pass MTD_IF_TYPE_SPI_NOR
interface type to create mtd device for spinor devices.

So If we add this design to SPI-NAND changes, we need to implement
- MTD dm core that can driver all interfaces

- one driver for raw nand

- one driver for spinand

- spi-mem

- convert fsl-qspi to spi-mem

- implement command to handle

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

+Richard to comment on the MTD abstraction stuff and how uboot port of UBI might be impacted by some changes requested here.

Hi Jagan,

Quoted 19 lines · On Mon, 25 Jun 2018 13:59:37 +0530 Jagan Teki <[email protected]> wrote:

I've looked the code on the respective patches, look like most of the code copy from Linux by adding __UBOOT__. I have no issue with Linux copy but we need to structure the code according to U-Boot in the form of driver-model (this series lack with that).

Here are my suggestions, based the MTD work so-far

First we need to design MTD driver-model which can capable to drive one driver from each interface. (not converting all interface drivers at once, that is taking more time and other issues)

Like Linux MTD, U-Boot should have MTD dm for underlying flash devices like nand, parallel nor, spinor etc. So to drive this theory with driver model(with an example of block layer) mtd is common device interaction for most of memory technology flashes like nand, parallel nor, spinor etc, these are treated as interface types wrt u-boot driver model.

Quoted 8 lines

Once the respective interface driver bind happen, the uclass driver will pass an 'interface type' to mtd layer to create device for it, for example once spinor ULASS_SPI_NOR driver bind happen, the uclass driver of spinor will pass MTD_IF_TYPE_SPI_NOR
interface type to create mtd device for spinor devices.

So If we add this design to SPI-NAND changes, we need to implement
- MTD dm core that can driver all interfaces

That's already what the MTD framework provides, and Miquel even added some stuff to integrate the MTD layer even further in the DM. It's probably not perfect yet, but the changes are, IMHO, going in the right direction.

Now, if you're talking about the new MTD API that creates helper functions prefixed with dm_, sorry, but I don't see the point. We already have plenty of MTD users in u-boot, they all manipulate MTD objects and go through the standard MTD API to do that. What you suggest would make things messier for several reasons:

1/ we won't be able to easily port Linux code to u-boot. Look at the
JFFS2 UBI support. They all use mtd_info objects. What's the point of changing that except making things harder to port.

2/ Not all MTD providers will be converted to the device model at once,
so how do you plan to deal with that?

3/ What's the benefit of exposing yet another way to manipulate MTD
devices?

Quoted 1 line

- one driver for raw nand

Unfortunately, that's not how it works right now, and clearly, we don't have time to work on this raw NAND rework right now.

Quoted 1 line

- one driver for spinand

I think that's already the case.

Quoted 1 line

- spi-mem

It's also what Miquel is doing in this series.

Quoted 1 line

- convert fsl-qspi to spi-mem

We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point.

Quoted 1 line

- implement command to handle

This I don't get. What do you mean by "implement command to handle"? Are we talking about cmd/mtd.c? I think the work Miquel has done is already a good start, what's missing in there?

Quoted 9 lines

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction:
- addition of DM helpers to the MTD_UCLASS
- addition of the spi-mem interface properly integrated in the DM model of the SPI framework
- addition of a SPI NAND driver, again properly integrated in the DM
- integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set

I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example).

I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide.

+Richard to comment on the MTD abstraction stuff and how uboot port of UBI might be impacted by some changes requested here.

Hi Jagan,

Quoted 19 lines · On Mon, 25 Jun 2018 13:59:37 +0530 Jagan Teki <[email protected]> wrote:

I've looked the code on the respective patches, look like most of the code copy from Linux by adding __UBOOT__. I have no issue with Linux copy but we need to structure the code according to U-Boot in the form of driver-model (this series lack with that).

Here are my suggestions, based the MTD work so-far

First we need to design MTD driver-model which can capable to drive one driver from each interface. (not converting all interface drivers at once, that is taking more time and other issues)

Like Linux MTD, U-Boot should have MTD dm for underlying flash devices like nand, parallel nor, spinor etc. So to drive this theory with driver model(with an example of block layer) mtd is common device interaction for most of memory technology flashes like nand, parallel nor, spinor etc, these are treated as interface types wrt u-boot driver model.

Quoted 8 lines

Once the respective interface driver bind happen, the uclass driver will pass an 'interface type' to mtd layer to create device for it, for example once spinor ULASS_SPI_NOR driver bind happen, the uclass driver of spinor will pass MTD_IF_TYPE_SPI_NOR
interface type to create mtd device for spinor devices.

So If we add this design to SPI-NAND changes, we need to implement
- MTD dm core that can driver all interfaces

That's already what the MTD framework provides, and Miquel even added some stuff to integrate the MTD layer even further in the DM. It's probably not perfect yet, but the changes are, IMHO, going in the right direction.

Now, if you're talking about the new MTD API that creates helper functions prefixed with dm_, sorry, but I don't see the point. We already have plenty of MTD users in u-boot, they all manipulate MTD objects and go through the standard MTD API to do that. What you suggest would make things messier for several reasons:

1/ we won't be able to easily port Linux code to u-boot. Look at the
JFFS2 UBI support. They all use mtd_info objects. What's the point of changing that except making things harder to port.

2/ Not all MTD providers will be converted to the device model at once,
so how do you plan to deal with that?

3/ What's the benefit of exposing yet another way to manipulate MTD
devices?

Quoted 1 line

- one driver for raw nand

Unfortunately, that's not how it works right now, and clearly, we don't have time to work on this raw NAND rework right now.

Quoted 1 line

- one driver for spinand

I think that's already the case.

Quoted 1 line

- spi-mem

It's also what Miquel is doing in this series.

Quoted 1 line

- convert fsl-qspi to spi-mem

We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point.

Quoted 2 lines

- implement command to handle

This I don't get. What do you mean by "implement command to handle"? Are we talking about cmd/mtd.c? I think the work Miquel has done is already a good start, what's missing in there?

Quoted 9 lines

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction:
- addition of DM helpers to the MTD_UCLASS
- addition of the spi-mem interface properly integrated in the DM model of the SPI framework
- addition of a SPI NAND driver, again properly integrated in the DM
- integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set

I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example).

I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide.

+ Simon
+ Tom
(suggesting MTD driver model abstraction layer)

Quoted 68 lines · Boris Brezillon

+Richard to comment on the MTD abstraction stuff and how uboot port of UBI might be impacted by some changes requested here.

Hi Jagan,

Quoted 19 lines · On Mon, 25 Jun 2018 13:59:37 +0530 Jagan Teki <[email protected]> wrote:

I've looked the code on the respective patches, look like most of the code copy from Linux by adding __UBOOT__. I have no issue with Linux copy but we need to structure the code according to U-Boot in the form of driver-model (this series lack with that).

Here are my suggestions, based the MTD work so-far

First we need to design MTD driver-model which can capable to drive one driver from each interface. (not converting all interface drivers at once, that is taking more time and other issues)

Like Linux MTD, U-Boot should have MTD dm for underlying flash devices like nand, parallel nor, spinor etc. So to drive this theory with driver model(with an example of block layer) mtd is common device interaction for most of memory technology flashes like nand, parallel nor, spinor etc, these are treated as interface types wrt u-boot driver model.

Quoted 8 lines

Once the respective interface driver bind happen, the uclass driver will pass an 'interface type' to mtd layer to create device for it, for example once spinor ULASS_SPI_NOR driver bind happen, the uclass driver of spinor will pass MTD_IF_TYPE_SPI_NOR
interface type to create mtd device for spinor devices.

So If we add this design to SPI-NAND changes, we need to implement
- MTD dm core that can driver all interfaces

That's already what the MTD framework provides, and Miquel even added some stuff to integrate the MTD layer even further in the DM. It's probably not perfect yet, but the changes are, IMHO, going in the right direction.

Now, if you're talking about the new MTD API that creates helper functions prefixed with dm_, sorry, but I don't see the point. We already have plenty of MTD users in u-boot, they all manipulate MTD objects and go through the standard MTD API to do that. What you suggest would make things messier for several reasons:

1/ we won't be able to easily port Linux code to u-boot. Look at the
JFFS2 UBI support. They all use mtd_info objects. What's the point of changing that except making things harder to port.

2/ Not all MTD providers will be converted to the device model at once,
so how do you plan to deal with that?

3/ What's the benefit of exposing yet another way to manipulate MTD
devices?

Quoted 1 line

- one driver for raw nand

Unfortunately, that's not how it works right now, and clearly, we don't have time to work on this raw NAND rework right now.

Quoted 1 line

- one driver for spinand

I think that's already the case.

Quoted 1 line

- spi-mem

Quoted 7 lines

It's also what Miquel is doing in this series.

Quoted 1 line

- convert fsl-qspi to spi-mem

We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point.

Can you point me that simple spi-mem controller driver?

Quoted 27 lines

Quoted 2 lines

- implement command to handle

This I don't get. What do you mean by "implement command to handle"? Are we talking about cmd/mtd.c? I think the work Miquel has done is already a good start, what's missing in there?

Quoted 9 lines

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction:
- addition of DM helpers to the MTD_UCLASS
- addition of the spi-mem interface properly integrated in the DM model of the SPI framework
- addition of a SPI NAND driver, again properly integrated in the DM
- integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set

Quoted 7 lines

I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example).

I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide.

To answer all these questions, I think we need to decide whether we go for MTD dm abstraction or existing MTD layer.

When I say MTD dm abstraction, all mtd operation prototypes are in the form of udevice unlike existing MTD has mtd_info. when I initially supporting spi-nor (during Linux early spi-nor) I've reused existing MTD and written something like what Miquel did using mtd_info ops [3]. but then developers on ML, proposed the new drivers should be form of driver-model abstraction, so I've added mtd driver model ops [4].

I understand the new MTD dm abstraction in U-Boot is not possible for direct syncing from Linux, but we really want the u-boot way of handling drivers and trying to copy code from Linux by removing __UBOOT__ or any Linux specific macros. Since this is pretty much big task, ie the reason I'm asking for single driver from each MTD device so-that once the clear stack is ready other drivers can convert one-by-one.

[3] http://git.denx.de/?p=u-boot-spi.git;a=commitdiff;h=d297949cd3f44278f109dff42fb88a879722121c
[4] https://patchwork.ozlabs.org/patch/853337/

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

- convert fsl-qspi to spi-mem

We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point.

Can you point me that simple spi-mem controller driver?

Quoted 38 lines

Quoted 20 lines

Quoted 9 lines

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction:
- addition of DM helpers to the MTD_UCLASS
- addition of the spi-mem interface properly integrated in the DM model of the SPI framework
- addition of a SPI NAND driver, again properly integrated in the DM
- integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set

Quoted 7 lines

I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example).

I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide.

To answer all these questions, I think we need to decide whether we go for MTD dm abstraction or existing MTD layer.

When I say MTD dm abstraction, all mtd operation prototypes are in the form of udevice unlike existing MTD has mtd_info. when I initially supporting spi-nor (during Linux early spi-nor) I've reused existing MTD and written something like what Miquel did using mtd_info ops [3]. but then developers on ML, proposed the new drivers should be form of driver-model abstraction, so I've added mtd driver model ops [4].

Quoted 7 lines

I understand the new MTD dm abstraction in U-Boot is not possible for direct syncing from Linux, but we really want the u-boot way of handling drivers and trying to copy code from Linux by removing __UBOOT__ or any Linux specific macros. Since this is pretty much big task, ie the reason I'm asking for single driver from each MTD device so-that once the clear stack is ready other drivers can convert one-by-one.

Quoted 7 lines · On Mon, 25 Jun 2018 19:58:37 +0530 Jagan Teki <[email protected]>…

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

- convert fsl-qspi to spi-mem

We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point.

Can you point me that simple spi-mem controller driver?

It's not a controller that implements spi-mem operations but a simple SPI controller (one that knows how to do regular SPI transfers and nothing more). But the spi-mem layer knows how to send spi_mem_op using regular transfer and that's why it works without any modification at the driver level.

Quoted 38 lines

Quoted 38 lines

Quoted 20 lines

Quoted 9 lines

For spi-nor interface design, we have an example code here[2]

I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we can add spi-mem, spi-nand on top of it or we can work together to convert them all.

Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction:
- addition of DM helpers to the MTD_UCLASS
- addition of the spi-mem interface properly integrated in the DM model of the SPI framework
- addition of a SPI NAND driver, again properly integrated in the DM
- integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set

Quoted 7 lines

I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example).

I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide.

To answer all these questions, I think we need to decide whether we go for MTD dm abstraction or existing MTD layer.

When I say MTD dm abstraction, all mtd operation prototypes are in the form of udevice unlike existing MTD has mtd_info. when I initially supporting spi-nor (during Linux early spi-nor) I've reused existing MTD and written something like what Miquel did using mtd_info ops [3]. but then developers on ML, proposed the new drivers should be form of driver-model abstraction, so I've added mtd driver model ops [4].

Quoted 7 lines

Quoted 7 lines

I understand the new MTD dm abstraction in U-Boot is not possible for direct syncing from Linux, but we really want the u-boot way of handling drivers and trying to copy code from Linux by removing __UBOOT__ or any Linux specific macros. Since this is pretty much big task, ie the reason I'm asking for single driver from each MTD device so-that once the clear stack is ready other drivers can convert one-by-one.