All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/4] Introduce mtdblock device
@ 2024-02-27 10:04 Alexey Romanov
  2024-02-27 10:04 ` [PATCH v1 1/4] drivers: introduce mtdblock abstraction Alexey Romanov
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Alexey Romanov @ 2024-02-27 10:04 UTC (permalink / raw)
  To: dario.binacchi, michael, frieder.schrempf, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot, Alexey Romanov

Hello!

This series adds support for the mtdblock device, which
allows to read/write data block by block. For example,
it can now be used for BCB or Android AB command:

  $ bcb load mtd 0 part_name

Tested only on SPI NAND, so bind is made only for
SPI NAND drivers.

Alexey Romanov (4):
  drivers: introduce mtdblock abstraction
  disk: support MTD partitions
  spinand: bind mtdblock
  efi: block: compile only if CONFIG_EFI_PARTITION enabled

 disk/part.c                 |   5 +-
 drivers/block/blk-uclass.c  |   1 +
 drivers/mtd/Kconfig         |   1 +
 drivers/mtd/Makefile        |   1 +
 drivers/mtd/mtdblock.c      | 170 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdpart.c       |  69 +++++++++++++++
 drivers/mtd/nand/spi/core.c |  20 +++++
 include/linux/mtd/mtd.h     |  12 +++
 include/part.h              |   2 +
 lib/efi_driver/Makefile     |   2 +-
 10 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mtd/mtdblock.c

-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v1 1/4] drivers: introduce mtdblock abstraction
  2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
@ 2024-02-27 10:04 ` Alexey Romanov
  2024-02-27 13:37   ` Heinrich Schuchardt
  2024-02-27 10:04 ` [PATCH v1 2/4] disk: support MTD partitions Alexey Romanov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Alexey Romanov @ 2024-02-27 10:04 UTC (permalink / raw)
  To: dario.binacchi, michael, frieder.schrempf, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot, Alexey Romanov

MTD block - abstraction over MTD subsystem, allowing
to read and write in blocks using BLK UCLASS.

- Read algorithm:

  1. Convert start block number to start address.
  2. Read block_dev->blksz bytes using mtd_read() and
     add to start address pointer block_dev->blksz bytes,
     until the requested number of blocks have been read.

- Write algorithm:

  1. Convert start block number to start address.
  2. Round this address down by mtd->erasesize.

  Erase addr      Start addr
     |                |
     v                v
     +----------------+----------------+----------------+
     |     blksz      |      blksz     |      blksz     |
     +----------------+----------------+----------------+

  3. Calculate offset between this two addresses.
  4. Read mtd->erasesize bytes using mtd_read() into
     temporary buffer from erase address.

  Erase addr      Start addr
     |                |
     v                v
     +----------------+----------------+----------------+
     |     blksz      |      blksz     |      blksz     |
     +----------------+----------------+----------------+
     ^            mtd->erasesize = blksz * 3
     |
     |
     |

  mtd_read()
  from here

  5. Copy data from user buffer to temporary buffer with offset,
     calculated at step 3.
  6. Erase and write mtd->erasesize bytes at erase address
     pointer using mtd_erase/mtd_write().
  7. Add to erase address pointer mtd->erasesize bytes.
  8. goto 1 until the requested number of blocks have
     been written.

Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
---
 drivers/block/blk-uclass.c |   1 +
 drivers/mtd/Makefile       |   1 +
 drivers/mtd/mtdblock.c     | 170 +++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h    |  12 +++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/mtd/mtdblock.c

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 77066da352..ab0a9105c9 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -37,6 +37,7 @@ static struct {
 	{ UCLASS_PVBLOCK, "pvblock" },
 	{ UCLASS_BLKMAP, "blkmap" },
 	{ UCLASS_RKMTD, "rkmtd" },
+	{ UCLASS_MTD, "mtd" },
 };
 
 static enum uclass_id uclass_name_to_iftype(const char *uclass_idname)
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index c638980ea2..993b122ac4 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -26,6 +26,7 @@ obj-y += onenand/
 obj-y += spi/
 obj-$(CONFIG_MTD_UBI) += ubi/
 obj-$(CONFIG_NVMXIP) += nvmxip/
+obj-$(CONFIG_BLK) += mtdblock.o
 
 #SPL/TPL build
 else
diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
new file mode 100644
index 0000000000..0563d0b795
--- /dev/null
+++ b/drivers/mtd/mtdblock.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2024 SaluteDevices, Inc.
+ *
+ * Author: Alexey Romanov <avromanov@salutedevices.com>
+ */
+
+#include <blk.h>
+#include <dm/device.h>
+#include <dm/device-internal.h>
+#include <linux/mtd/mtd.h>
+
+int mtd_bind(struct udevice *dev, struct mtd_info **mtd)
+{
+	struct blk_desc *bdesc;
+	struct udevice *bdev;
+	int ret;
+
+	ret = blk_create_devicef(dev, "mtd_blk", "blk", UCLASS_MTD,
+				 dev_seq(dev), 512, 0, &bdev);
+	if (ret) {
+		pr_err("Cannot create block device");
+		return ret;
+	}
+
+	bdesc = dev_get_uclass_plat(bdev);
+	dev_set_priv(bdev, mtd);
+	bdesc->bdev = bdev;
+
+	return 0;
+}
+
+static ulong mtd_bread(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
+		       void *dst)
+{
+	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
+	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
+	unsigned int sect_size = block_dev->blksz;
+	lbaint_t cur = start;
+	ulong read_cnt = 0;
+
+	while (read_cnt < blkcnt) {
+		int ret;
+		loff_t sect_start = cur * sect_size;
+		size_t retlen;
+
+		ret = mtd_read(mtd, sect_start, sect_size, &retlen, dst);
+		if (ret)
+			return ret;
+
+		if (retlen != sect_size) {
+			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
+			return -EIO;
+		}
+
+		cur++;
+		dst += sect_size;
+		read_cnt++;
+	}
+
+	return read_cnt;
+}
+
+static int mtd_erase_write(struct mtd_info *mtd, uint64_t start, const void *src)
+{
+	int ret;
+	size_t retlen;
+	struct erase_info erase = { 0 };
+
+	erase.mtd = mtd;
+	erase.addr = start;
+	erase.len = mtd->erasesize;
+
+	ret = mtd_erase(mtd, &erase);
+	if (ret)
+		return ret;
+
+	ret = mtd_write(mtd, start, mtd->erasesize, &retlen, src);
+	if (ret)
+		return ret;
+
+	if (retlen != mtd->erasesize) {
+		pr_err("mtdblock: failed to read block at 0x%llx\n", start);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static ulong mtd_bwrite(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
+			const void *src)
+{
+	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
+	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
+	unsigned int sect_size = block_dev->blksz;
+	lbaint_t cur = start, blocks_todo = blkcnt;
+	ulong write_cnt = 0;
+	u8 *buf;
+	int ret = 0;
+
+	buf = malloc(mtd->erasesize);
+	if (!buf)
+		return -ENOMEM;
+
+	while (blocks_todo > 0) {
+		loff_t sect_start = cur * sect_size;
+		loff_t erase_start = ALIGN_DOWN(sect_start, mtd->erasesize);
+		u32 offset = sect_start - erase_start;
+		size_t cur_size = min_t(size_t,  mtd->erasesize - offset,
+					blocks_todo * sect_size);
+		size_t retlen;
+		lbaint_t written;
+
+		ret = mtd_read(mtd, erase_start, mtd->erasesize, &retlen, buf);
+		if (ret)
+			goto out;
+
+		if (retlen != mtd->erasesize) {
+			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
+			ret = -EIO;
+			goto out;
+		}
+
+		memcpy(buf + offset, src, cur_size);
+
+		ret = mtd_erase_write(mtd, erase_start, buf);
+		if (ret)
+			goto out;
+
+		written = cur_size / sect_size;
+
+		blocks_todo -= written;
+		cur += written;
+		src += cur_size;
+		write_cnt += written;
+	}
+
+out:
+	free(buf);
+
+	if (ret)
+		return ret;
+
+	return write_cnt;
+}
+
+static int mtd_blk_probe(struct udevice *dev)
+{
+	int ret;
+
+	ret = device_probe(dev);
+	if (ret) {
+		pr_err("Probing %s failed (err=%d)\n", dev->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct blk_ops mtd_blk_ops = {
+	.read = mtd_bread,
+	.write = mtd_bwrite,
+};
+
+U_BOOT_DRIVER(mtd_blk) = {
+	.name = "mtd_blk",
+	.id = UCLASS_BLK,
+	.ops = &mtd_blk_ops,
+	.probe = mtd_blk_probe,
+};
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 09f5269887..9b997fadd1 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -26,6 +26,9 @@
 #include <dm/device.h>
 #endif
 #include <dm/ofnode.h>
+#if IS_ENABLED(CONFIG_BLK)
+#include <blk.h>
+#endif
 
 #define MAX_MTD_DEVICES 32
 #endif
@@ -412,6 +415,15 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		    const u_char *buf);
 
+#if IS_ENABLED(CONFIG_BLK)
+static inline struct mtd_info *blk_desc_to_mtd(struct blk_desc *bdesc)
+{
+	return *((struct mtd_info **)dev_get_priv(bdesc->bdev));
+}
+
+int mtd_bind(struct udevice *dev, struct mtd_info **mtd);
+#endif
+
 int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
 int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 2/4] disk: support MTD partitions
  2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
  2024-02-27 10:04 ` [PATCH v1 1/4] drivers: introduce mtdblock abstraction Alexey Romanov
@ 2024-02-27 10:04 ` Alexey Romanov
  2024-02-27 13:28   ` Heinrich Schuchardt
  2024-02-27 10:04 ` [PATCH v1 3/4] spinand: bind mtdblock Alexey Romanov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Alexey Romanov @ 2024-02-27 10:04 UTC (permalink / raw)
  To: dario.binacchi, michael, frieder.schrempf, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot, Alexey Romanov

Add new MTD partition driver, which can be useful with
mtdblock driver combination.

Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
---
 disk/part.c           |  5 +++-
 drivers/mtd/Kconfig   |  1 +
 drivers/mtd/mtdpart.c | 69 +++++++++++++++++++++++++++++++++++++++++++
 include/part.h        |  2 ++
 4 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/disk/part.c b/disk/part.c
index 36b88205ec..0fc5cc0419 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -304,7 +304,8 @@ static void print_part_header(const char *type, struct blk_desc *desc)
 	CONFIG_IS_ENABLED(DOS_PARTITION) || \
 	CONFIG_IS_ENABLED(ISO_PARTITION) || \
 	CONFIG_IS_ENABLED(AMIGA_PARTITION) || \
-	CONFIG_IS_ENABLED(EFI_PARTITION)
+	CONFIG_IS_ENABLED(EFI_PARTITION) || \
+	CONFIG_IS_ENABLED(MTD_PARTITIONS)
 	puts ("\nPartition Map for ");
 	switch (desc->uclass_id) {
 	case UCLASS_IDE:
@@ -343,6 +344,8 @@ static void print_part_header(const char *type, struct blk_desc *desc)
 	case UCLASS_BLKMAP:
 		puts("BLKMAP");
 		break;
+	case UCLASS_MTD:
+		puts("MTD");
 	default:
 		printf("UNKNOWN(%d)", desc->uclass_id);
 		break;
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 1902351719..40272f7e50 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -2,6 +2,7 @@ menu "MTD Support"
 
 config MTD_PARTITIONS
 	bool
+	select PARTITIONS
 
 config MTD
 	bool "Enable MTD layer"
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 4886392a1c..608908c193 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -21,6 +21,8 @@
 
 #include <common.h>
 #include <malloc.h>
+#include <memalign.h>
+#include <part.h>
 #include <linux/bug.h>
 #include <linux/errno.h>
 #include <linux/compat.h>
@@ -1055,3 +1057,70 @@ uint64_t mtd_get_device_size(const struct mtd_info *mtd)
 	return mtd->size;
 }
 EXPORT_SYMBOL_GPL(mtd_get_device_size);
+
+static struct mtd_info *mtd_get_partition_by_index(struct mtd_info *mtd, int index)
+{
+	struct mtd_info *part;
+	int i = 0;
+
+	list_for_each_entry(part, &mtd->partitions, node)
+		if (i++ == index)
+			return part;
+
+	debug("Partition with idx=%d not found on MTD device %s\n", index, mtd->name);
+	return NULL;
+}
+
+static int __maybe_unused part_get_info_mtd(struct blk_desc *dev_desc, int part_idx,
+					    struct disk_partition *info)
+{
+	struct mtd_info *master = blk_desc_to_mtd(dev_desc);
+	struct mtd_info *part;
+
+	if (!master) {
+		pr_err("MTD device is NULL\n");
+		return -EINVAL;
+	}
+
+	part = mtd_get_partition_by_index(master, part_idx);
+	if (!part) {
+		debug("Failed to find partition with idx=%d\n", part_idx);
+		return -EINVAL;
+	}
+
+	snprintf(info->name, PART_NAME_LEN, part->name);
+	info->start = part->offset / dev_desc->blksz;
+	info->size = part->size / dev_desc->blksz;
+	info->blksz = dev_desc->blksz;
+
+	return 0;
+}
+
+static void __maybe_unused part_print_mtd(struct blk_desc *dev_desc)
+{
+	struct mtd_info *master = blk_desc_to_mtd(dev_desc);
+	struct mtd_info *part;
+
+	list_for_each_entry(part, &master->partitions, node)
+		printf("- 0x%012llx-0x%012llx : \"%s\"\n",
+		       part->offset, part->offset + part->size, part->name);
+}
+
+static int part_test_mtd(struct blk_desc *dev_desc)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
+
+	if (blk_dread(dev_desc, 0, 1, (ulong *)buffer) != 1)
+		return -1;
+
+	return 0;
+}
+
+U_BOOT_PART_TYPE(mtd) = {
+	.name		= "MTD",
+	.part_type	= PART_TYPE_MTD,
+	.max_entries	= MTD_ENTRY_NUMBERS,
+	.get_info	= part_get_info_ptr(part_get_info_mtd),
+	.print		= part_print_ptr(part_print_mtd),
+	.test		= part_test_mtd,
+};
diff --git a/include/part.h b/include/part.h
index db34bc6bb7..f7f3773a95 100644
--- a/include/part.h
+++ b/include/part.h
@@ -30,12 +30,14 @@ struct block_drvr {
 #define PART_TYPE_ISO		0x03
 #define PART_TYPE_AMIGA		0x04
 #define PART_TYPE_EFI		0x05
+#define PART_TYPE_MTD		0x06
 
 /* maximum number of partition entries supported by search */
 #define DOS_ENTRY_NUMBERS	8
 #define ISO_ENTRY_NUMBERS	64
 #define MAC_ENTRY_NUMBERS	64
 #define AMIGA_ENTRY_NUMBERS	8
+#define MTD_ENTRY_NUMBERS	64
 /*
  * Type string for U-Boot bootable partitions
  */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 3/4] spinand: bind mtdblock
  2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
  2024-02-27 10:04 ` [PATCH v1 1/4] drivers: introduce mtdblock abstraction Alexey Romanov
  2024-02-27 10:04 ` [PATCH v1 2/4] disk: support MTD partitions Alexey Romanov
@ 2024-02-27 10:04 ` Alexey Romanov
  2024-02-27 13:23   ` Heinrich Schuchardt
  2024-02-27 10:04 ` [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled Alexey Romanov
  2024-02-29  8:51 ` [PATCH v1 0/4] Introduce mtdblock device Frieder Schrempf
  4 siblings, 1 reply; 13+ messages in thread
From: Alexey Romanov @ 2024-02-27 10:04 UTC (permalink / raw)
  To: dario.binacchi, michael, frieder.schrempf, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot, Alexey Romanov

Bind SPI-NAND driver to MTD block driver.

Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
---
 drivers/mtd/nand/spi/core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 2a3dbcfcb4..1d9cf66e4a 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -36,6 +36,10 @@
 #include <linux/printk.h>
 #endif
 
+struct spinand_plat {
+	struct mtd_info *mtd;
+};
+
 /* SPI NAND index visible in MTD names */
 static int spi_nand_idx;
 
@@ -1174,12 +1178,22 @@ static void spinand_cleanup(struct spinand_device *spinand)
 	kfree(spinand->scratchbuf);
 }
 
+#if CONFIG_IS_ENABLED(BLK)
+static int spinand_bind(struct udevice *dev)
+{
+	struct spinand_plat *plat = dev_get_plat(dev);
+
+	return mtd_bind(dev, &plat->mtd);
+}
+#endif
+
 static int spinand_probe(struct udevice *dev)
 {
 	struct spinand_device *spinand = dev_get_priv(dev);
 	struct spi_slave *slave = dev_get_parent_priv(dev);
 	struct mtd_info *mtd = dev_get_uclass_priv(dev);
 	struct nand_device *nand = spinand_to_nand(spinand);
+	struct spinand_plat *plat = dev_get_plat(dev);
 	int ret;
 
 #ifndef __UBOOT__
@@ -1219,6 +1233,8 @@ static int spinand_probe(struct udevice *dev)
 	if (ret)
 		goto err_spinand_cleanup;
 
+	plat->mtd = mtd;
+
 	return 0;
 
 err_spinand_cleanup:
@@ -1288,6 +1304,10 @@ U_BOOT_DRIVER(spinand) = {
 	.of_match = spinand_ids,
 	.priv_auto	= sizeof(struct spinand_device),
 	.probe = spinand_probe,
+#if CONFIG_IS_ENABLED(BLK)
+	.bind = spinand_bind,
+#endif
+	.plat_auto = sizeof(struct spinand_plat),
 };
 
 void board_nand_init(void)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled
  2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
                   ` (2 preceding siblings ...)
  2024-02-27 10:04 ` [PATCH v1 3/4] spinand: bind mtdblock Alexey Romanov
@ 2024-02-27 10:04 ` Alexey Romanov
  2024-02-27 13:18   ` Heinrich Schuchardt
  2024-02-29  8:51 ` [PATCH v1 0/4] Introduce mtdblock device Frieder Schrempf
  4 siblings, 1 reply; 13+ messages in thread
From: Alexey Romanov @ 2024-02-27 10:04 UTC (permalink / raw)
  To: dario.binacchi, michael, frieder.schrempf, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot, Alexey Romanov

We have to compile efi_block abstraction only if option
EFI_PARTITION is enabled. For example, if the user
only enabled MTD_PARTITIONS, we would still compile
efi_block. This is incorrect.

Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
---
 lib/efi_driver/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_driver/Makefile b/lib/efi_driver/Makefile
index f2b6c05cc2..1fc85ce6f9 100644
--- a/lib/efi_driver/Makefile
+++ b/lib/efi_driver/Makefile
@@ -6,6 +6,6 @@
 # object inclusion implicitly depends on it
 
 obj-y += efi_uclass.o
-ifeq ($(CONFIG_PARTITIONS),y)
+ifeq ($(CONFIG_EFI_PARTITION),y)
 obj-y += efi_block_device.o
 endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled
  2024-02-27 10:04 ` [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled Alexey Romanov
@ 2024-02-27 13:18   ` Heinrich Schuchardt
  0 siblings, 0 replies; 13+ messages in thread
From: Heinrich Schuchardt @ 2024-02-27 13:18 UTC (permalink / raw)
  To: Alexey Romanov
  Cc: kernel, u-boot, dario.binacchi, michael, frieder.schrempf,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard

On 27.02.24 11:04, Alexey Romanov wrote:
> We have to compile efi_block abstraction only if option
> EFI_PARTITION is enabled. For example, if the user
> only enabled MTD_PARTITIONS, we would still compile
> efi_block. This is incorrect.
>
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>   lib/efi_driver/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_driver/Makefile b/lib/efi_driver/Makefile
> index f2b6c05cc2..1fc85ce6f9 100644
> --- a/lib/efi_driver/Makefile
> +++ b/lib/efi_driver/Makefile
> @@ -6,6 +6,6 @@
>   # object inclusion implicitly depends on it
>
>   obj-y += efi_uclass.o
> -ifeq ($(CONFIG_PARTITIONS),y)
> +ifeq ($(CONFIG_EFI_PARTITION),y)

The symbol CONFIG_EFI_PARTITION is not related to UEFI. It provides
support for GPT partition tables.

efi_block_device.c provides support for block devices provided by EFI
applications. This should not depend on GPT partition table support
(EFI_PARTITION) as it well usable with other partition tables too.

I would not know why iPXE loaded from an MTD block device should not be
able allowed to provide an iSCSI drive.

Please, drop this patch.

Best regards

Heinrich

>   obj-y += efi_block_device.o
>   endif


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 3/4] spinand: bind mtdblock
  2024-02-27 10:04 ` [PATCH v1 3/4] spinand: bind mtdblock Alexey Romanov
@ 2024-02-27 13:23   ` Heinrich Schuchardt
  0 siblings, 0 replies; 13+ messages in thread
From: Heinrich Schuchardt @ 2024-02-27 13:23 UTC (permalink / raw)
  To: Alexey Romanov
  Cc: kernel, u-boot, dario.binacchi, michael, frieder.schrempf,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard

On 27.02.24 11:04, Alexey Romanov wrote:
> Bind SPI-NAND driver to MTD block driver.
>
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>   drivers/mtd/nand/spi/core.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 2a3dbcfcb4..1d9cf66e4a 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -36,6 +36,10 @@
>   #include <linux/printk.h>
>   #endif
>
> +struct spinand_plat {
> +	struct mtd_info *mtd;
> +};
> +
>   /* SPI NAND index visible in MTD names */
>   static int spi_nand_idx;
>
> @@ -1174,12 +1178,22 @@ static void spinand_cleanup(struct spinand_device *spinand)
>   	kfree(spinand->scratchbuf);
>   }
>
> +#if CONFIG_IS_ENABLED(BLK)
> +static int spinand_bind(struct udevice *dev)
> +{
> +	struct spinand_plat *plat = dev_get_plat(dev);
> +
> +	return mtd_bind(dev, &plat->mtd);
> +}
> +#endif
> +
>   static int spinand_probe(struct udevice *dev)
>   {
>   	struct spinand_device *spinand = dev_get_priv(dev);
>   	struct spi_slave *slave = dev_get_parent_priv(dev);
>   	struct mtd_info *mtd = dev_get_uclass_priv(dev);
>   	struct nand_device *nand = spinand_to_nand(spinand);
> +	struct spinand_plat *plat = dev_get_plat(dev);
>   	int ret;
>
>   #ifndef __UBOOT__
> @@ -1219,6 +1233,8 @@ static int spinand_probe(struct udevice *dev)
>   	if (ret)
>   		goto err_spinand_cleanup;
>
> +	plat->mtd = mtd;
> +
>   	return 0;
>
>   err_spinand_cleanup:
> @@ -1288,6 +1304,10 @@ U_BOOT_DRIVER(spinand) = {
>   	.of_match = spinand_ids,
>   	.priv_auto	= sizeof(struct spinand_device),
>   	.probe = spinand_probe,
> +#if CONFIG_IS_ENABLED(BLK)


Please, have a look at doc/develop/designprinciples.rst:168:

     * Avoid ``#ifdefs`` where possible

Please, use 'if CONFIG_IS_ENABLED(BLK)' in the bind function.

Best regards

Heinrich

> +	.bind = spinand_bind,
> +#endif
> +	.plat_auto = sizeof(struct spinand_plat),
>   };
>
>   void board_nand_init(void)


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] disk: support MTD partitions
  2024-02-27 10:04 ` [PATCH v1 2/4] disk: support MTD partitions Alexey Romanov
@ 2024-02-27 13:28   ` Heinrich Schuchardt
  0 siblings, 0 replies; 13+ messages in thread
From: Heinrich Schuchardt @ 2024-02-27 13:28 UTC (permalink / raw)
  To: Alexey Romanov
  Cc: kernel, u-boot, dario.binacchi, michael, frieder.schrempf,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard

On 27.02.24 11:04, Alexey Romanov wrote:
> Add new MTD partition driver, which can be useful with
> mtdblock driver combination.
>
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>   disk/part.c           |  5 +++-
>   drivers/mtd/Kconfig   |  1 +
>   drivers/mtd/mtdpart.c | 69 +++++++++++++++++++++++++++++++++++++++++++
>   include/part.h        |  2 ++
>   4 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 36b88205ec..0fc5cc0419 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -304,7 +304,8 @@ static void print_part_header(const char *type, struct blk_desc *desc)
>   	CONFIG_IS_ENABLED(DOS_PARTITION) || \
>   	CONFIG_IS_ENABLED(ISO_PARTITION) || \
>   	CONFIG_IS_ENABLED(AMIGA_PARTITION) || \
> -	CONFIG_IS_ENABLED(EFI_PARTITION)
> +	CONFIG_IS_ENABLED(EFI_PARTITION) || \
> +	CONFIG_IS_ENABLED(MTD_PARTITIONS)
>   	puts ("\nPartition Map for ");
>   	switch (desc->uclass_id) {
>   	case UCLASS_IDE:
> @@ -343,6 +344,8 @@ static void print_part_header(const char *type, struct blk_desc *desc)
>   	case UCLASS_BLKMAP:
>   		puts("BLKMAP");
>   		break;
> +	case UCLASS_MTD:
> +		puts("MTD");
>   	default:
>   		printf("UNKNOWN(%d)", desc->uclass_id);
>   		break;
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 1902351719..40272f7e50 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -2,6 +2,7 @@ menu "MTD Support"
>
>   config MTD_PARTITIONS
>   	bool
> +	select PARTITIONS
>
>   config MTD
>   	bool "Enable MTD layer"
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 4886392a1c..608908c193 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -21,6 +21,8 @@
>
>   #include <common.h>
>   #include <malloc.h>
> +#include <memalign.h>
> +#include <part.h>
>   #include <linux/bug.h>
>   #include <linux/errno.h>
>   #include <linux/compat.h>
> @@ -1055,3 +1057,70 @@ uint64_t mtd_get_device_size(const struct mtd_info *mtd)
>   	return mtd->size;
>   }
>   EXPORT_SYMBOL_GPL(mtd_get_device_size);
> +
> +static struct mtd_info *mtd_get_partition_by_index(struct mtd_info *mtd, int index)
> +{
> +	struct mtd_info *part;
> +	int i = 0;
> +
> +	list_for_each_entry(part, &mtd->partitions, node)
> +		if (i++ == index)
> +			return part;
> +
> +	debug("Partition with idx=%d not found on MTD device %s\n", index, mtd->name);
> +	return NULL;
> +}
> +
> +static int __maybe_unused part_get_info_mtd(struct blk_desc *dev_desc, int part_idx,
> +					    struct disk_partition *info)

You use the function in U_BOOT_PART_TYPE(mtd). In struct part_driver
there are no conditional fields. Why mark it as __maybe_unused?

> +{
> +	struct mtd_info *master = blk_desc_to_mtd(dev_desc);
> +	struct mtd_info *part;
> +
> +	if (!master) {
> +		pr_err("MTD device is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	part = mtd_get_partition_by_index(master, part_idx);
> +	if (!part) {
> +		debug("Failed to find partition with idx=%d\n", part_idx);
> +		return -EINVAL;
> +	}
> +
> +	snprintf(info->name, PART_NAME_LEN, part->name);
> +	info->start = part->offset / dev_desc->blksz;
> +	info->size = part->size / dev_desc->blksz;
> +	info->blksz = dev_desc->blksz;
> +
> +	return 0;
> +}
> +
> +static void __maybe_unused part_print_mtd(struct blk_desc *dev_desc)

ditto

Best regards

Heinrich

> +{
> +	struct mtd_info *master = blk_desc_to_mtd(dev_desc);
> +	struct mtd_info *part;
> +
> +	list_for_each_entry(part, &master->partitions, node)
> +		printf("- 0x%012llx-0x%012llx : \"%s\"\n",
> +		       part->offset, part->offset + part->size, part->name);
> +}
> +
> +static int part_test_mtd(struct blk_desc *dev_desc)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
> +
> +	if (blk_dread(dev_desc, 0, 1, (ulong *)buffer) != 1)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +U_BOOT_PART_TYPE(mtd) = {
> +	.name		= "MTD",
> +	.part_type	= PART_TYPE_MTD,
> +	.max_entries	= MTD_ENTRY_NUMBERS,
> +	.get_info	= part_get_info_ptr(part_get_info_mtd),
> +	.print		= part_print_ptr(part_print_mtd),
> +	.test		= part_test_mtd,
> +};
> diff --git a/include/part.h b/include/part.h
> index db34bc6bb7..f7f3773a95 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -30,12 +30,14 @@ struct block_drvr {
>   #define PART_TYPE_ISO		0x03
>   #define PART_TYPE_AMIGA		0x04
>   #define PART_TYPE_EFI		0x05
> +#define PART_TYPE_MTD		0x06
>
>   /* maximum number of partition entries supported by search */
>   #define DOS_ENTRY_NUMBERS	8
>   #define ISO_ENTRY_NUMBERS	64
>   #define MAC_ENTRY_NUMBERS	64
>   #define AMIGA_ENTRY_NUMBERS	8
> +#define MTD_ENTRY_NUMBERS	64
>   /*
>    * Type string for U-Boot bootable partitions
>    */


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/4] drivers: introduce mtdblock abstraction
  2024-02-27 10:04 ` [PATCH v1 1/4] drivers: introduce mtdblock abstraction Alexey Romanov
@ 2024-02-27 13:37   ` Heinrich Schuchardt
  2024-03-07 10:24     ` Alexey Romanov
  0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2024-02-27 13:37 UTC (permalink / raw)
  To: Alexey Romanov
  Cc: kernel, u-boot, dario.binacchi, michael, frieder.schrempf,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard

On 27.02.24 11:04, Alexey Romanov wrote:
> MTD block - abstraction over MTD subsystem, allowing
> to read and write in blocks using BLK UCLASS.
>
> - Read algorithm:
>
>    1. Convert start block number to start address.
>    2. Read block_dev->blksz bytes using mtd_read() and
>       add to start address pointer block_dev->blksz bytes,
>       until the requested number of blocks have been read.
>
> - Write algorithm:
>
>    1. Convert start block number to start address.
>    2. Round this address down by mtd->erasesize.
>
>    Erase addr      Start addr
>       |                |
>       v                v
>       +----------------+----------------+----------------+
>       |     blksz      |      blksz     |      blksz     |
>       +----------------+----------------+----------------+
>
>    3. Calculate offset between this two addresses.
>    4. Read mtd->erasesize bytes using mtd_read() into
>       temporary buffer from erase address.
>
>    Erase addr      Start addr
>       |                |
>       v                v
>       +----------------+----------------+----------------+
>       |     blksz      |      blksz     |      blksz     |
>       +----------------+----------------+----------------+
>       ^            mtd->erasesize = blksz * 3

Can you really erase only the start of the erase block?

Wouldn't you have also to round the end address up to the end of an
erase block and read and rewrite the data between the end address and
the end of the last erase block?

>       |
>       |
>       |
>
>    mtd_read()
>    from here
>
>    5. Copy data from user buffer to temporary buffer with offset,
>       calculated at step 3.
>    6. Erase and write mtd->erasesize bytes at erase address
>       pointer using mtd_erase/mtd_write().
>    7. Add to erase address pointer mtd->erasesize bytes.
>    8. goto 1 until the requested number of blocks have
>       been written.

Commit messages is not were developers typically look for documentation.
Please, add the information to /doc/develop/ or to the source code.

Best regards

Heinrich

>
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>   drivers/block/blk-uclass.c |   1 +
>   drivers/mtd/Makefile       |   1 +
>   drivers/mtd/mtdblock.c     | 170 +++++++++++++++++++++++++++++++++++++
>   include/linux/mtd/mtd.h    |  12 +++
>   4 files changed, 184 insertions(+)
>   create mode 100644 drivers/mtd/mtdblock.c
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 77066da352..ab0a9105c9 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -37,6 +37,7 @@ static struct {
>   	{ UCLASS_PVBLOCK, "pvblock" },
>   	{ UCLASS_BLKMAP, "blkmap" },
>   	{ UCLASS_RKMTD, "rkmtd" },
> +	{ UCLASS_MTD, "mtd" },
>   };
>
>   static enum uclass_id uclass_name_to_iftype(const char *uclass_idname)
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index c638980ea2..993b122ac4 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -26,6 +26,7 @@ obj-y += onenand/
>   obj-y += spi/
>   obj-$(CONFIG_MTD_UBI) += ubi/
>   obj-$(CONFIG_NVMXIP) += nvmxip/
> +obj-$(CONFIG_BLK) += mtdblock.o
>
>   #SPL/TPL build
>   else
> diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
> new file mode 100644
> index 0000000000..0563d0b795
> --- /dev/null
> +++ b/drivers/mtd/mtdblock.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2024 SaluteDevices, Inc.
> + *
> + * Author: Alexey Romanov <avromanov@salutedevices.com>
> + */
> +
> +#include <blk.h>
> +#include <dm/device.h>
> +#include <dm/device-internal.h>
> +#include <linux/mtd/mtd.h>
> +
> +int mtd_bind(struct udevice *dev, struct mtd_info **mtd)
> +{
> +	struct blk_desc *bdesc;
> +	struct udevice *bdev;
> +	int ret;
> +
> +	ret = blk_create_devicef(dev, "mtd_blk", "blk", UCLASS_MTD,
> +				 dev_seq(dev), 512, 0, &bdev);
> +	if (ret) {
> +		pr_err("Cannot create block device");
> +		return ret;
> +	}
> +
> +	bdesc = dev_get_uclass_plat(bdev);
> +	dev_set_priv(bdev, mtd);
> +	bdesc->bdev = bdev;
> +
> +	return 0;
> +}
> +
> +static ulong mtd_bread(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> +		       void *dst)
> +{
> +	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
> +	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
> +	unsigned int sect_size = block_dev->blksz;
> +	lbaint_t cur = start;
> +	ulong read_cnt = 0;
> +
> +	while (read_cnt < blkcnt) {
> +		int ret;
> +		loff_t sect_start = cur * sect_size;
> +		size_t retlen;
> +
> +		ret = mtd_read(mtd, sect_start, sect_size, &retlen, dst);
> +		if (ret)
> +			return ret;
> +
> +		if (retlen != sect_size) {
> +			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
> +			return -EIO;
> +		}
> +
> +		cur++;
> +		dst += sect_size;
> +		read_cnt++;
> +	}
> +
> +	return read_cnt;
> +}
> +
> +static int mtd_erase_write(struct mtd_info *mtd, uint64_t start, const void *src)
> +{
> +	int ret;
> +	size_t retlen;
> +	struct erase_info erase = { 0 };
> +
> +	erase.mtd = mtd;
> +	erase.addr = start;
> +	erase.len = mtd->erasesize;
> +
> +	ret = mtd_erase(mtd, &erase);
> +	if (ret)
> +		return ret;
> +
> +	ret = mtd_write(mtd, start, mtd->erasesize, &retlen, src);
> +	if (ret)
> +		return ret;
> +
> +	if (retlen != mtd->erasesize) {
> +		pr_err("mtdblock: failed to read block at 0x%llx\n", start);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ulong mtd_bwrite(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> +			const void *src)
> +{
> +	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
> +	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
> +	unsigned int sect_size = block_dev->blksz;
> +	lbaint_t cur = start, blocks_todo = blkcnt;
> +	ulong write_cnt = 0;
> +	u8 *buf;
> +	int ret = 0;
> +
> +	buf = malloc(mtd->erasesize);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	while (blocks_todo > 0) {
> +		loff_t sect_start = cur * sect_size;
> +		loff_t erase_start = ALIGN_DOWN(sect_start, mtd->erasesize);
> +		u32 offset = sect_start - erase_start;
> +		size_t cur_size = min_t(size_t,  mtd->erasesize - offset,
> +					blocks_todo * sect_size);
> +		size_t retlen;
> +		lbaint_t written;
> +
> +		ret = mtd_read(mtd, erase_start, mtd->erasesize, &retlen, buf);
> +		if (ret)
> +			goto out;
> +
> +		if (retlen != mtd->erasesize) {
> +			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
> +			ret = -EIO;
> +			goto out;
> +		}
> +
> +		memcpy(buf + offset, src, cur_size);
> +
> +		ret = mtd_erase_write(mtd, erase_start, buf);
> +		if (ret)
> +			goto out;
> +
> +		written = cur_size / sect_size;
> +
> +		blocks_todo -= written;
> +		cur += written;
> +		src += cur_size;
> +		write_cnt += written;
> +	}
> +
> +out:
> +	free(buf);
> +
> +	if (ret)
> +		return ret;
> +
> +	return write_cnt;
> +}
> +
> +static int mtd_blk_probe(struct udevice *dev)
> +{
> +	int ret;
> +
> +	ret = device_probe(dev);
> +	if (ret) {
> +		pr_err("Probing %s failed (err=%d)\n", dev->name, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct blk_ops mtd_blk_ops = {
> +	.read = mtd_bread,
> +	.write = mtd_bwrite,
> +};
> +
> +U_BOOT_DRIVER(mtd_blk) = {
> +	.name = "mtd_blk",
> +	.id = UCLASS_BLK,
> +	.ops = &mtd_blk_ops,
> +	.probe = mtd_blk_probe,
> +};
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 09f5269887..9b997fadd1 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -26,6 +26,9 @@
>   #include <dm/device.h>
>   #endif
>   #include <dm/ofnode.h>
> +#if IS_ENABLED(CONFIG_BLK)
> +#include <blk.h>
> +#endif
>
>   #define MAX_MTD_DEVICES 32
>   #endif
> @@ -412,6 +415,15 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>   int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>   		    const u_char *buf);
>
> +#if IS_ENABLED(CONFIG_BLK)
> +static inline struct mtd_info *blk_desc_to_mtd(struct blk_desc *bdesc)
> +{
> +	return *((struct mtd_info **)dev_get_priv(bdesc->bdev));
> +}
> +
> +int mtd_bind(struct udevice *dev, struct mtd_info **mtd);
> +#endif
> +
>   int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
>   int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 0/4] Introduce mtdblock device
  2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
                   ` (3 preceding siblings ...)
  2024-02-27 10:04 ` [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled Alexey Romanov
@ 2024-02-29  8:51 ` Frieder Schrempf
  2024-03-01 13:42   ` Alexey Romanov
  4 siblings, 1 reply; 13+ messages in thread
From: Frieder Schrempf @ 2024-02-29  8:51 UTC (permalink / raw)
  To: Alexey Romanov, dario.binacchi, michael, xypron.glpk,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard
  Cc: kernel, u-boot

Hi Alexey,

On 27.02.24 11:04, Alexey Romanov wrote:
> Hello!
> 
> This series adds support for the mtdblock device, which
> allows to read/write data block by block. For example,
> it can now be used for BCB or Android AB command:
> 
>   $ bcb load mtd 0 part_name
> 
> Tested only on SPI NAND, so bind is made only for
> SPI NAND drivers.

I don't know much about Android, but as far as I understand you are
trying to store dynamic, boot-related information on the MTD device.

This might be acceptable if the underlying device is a NOR flash, but
for any type of NAND device it sounds like a rather bad idea.

NAND devices need some kind of FTL (flash translation layer) in order to
work reliably. Using a block filesystem directly on the NAND MTD device
will cause problems as soon as any bad blocks or bit flips occur.

People are therefore discouraged to use mtdblock on NAND and in the
kernel there is even a warning if you try to mount a NAND mtdblock
partition.

Maybe you should reconsider this and look into how to use UBIFS as a
FTL. On the other hand I'm not quite sure if the UBIFS layer in U-Boot
is really stable and maintained. So this might not be a good idea either.

Anyway, I'm against implementing mtdblock for SPI NAND (or any other NAND).

Best regards
Frieder

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 0/4] Introduce mtdblock device
  2024-02-29  8:51 ` [PATCH v1 0/4] Introduce mtdblock device Frieder Schrempf
@ 2024-03-01 13:42   ` Alexey Romanov
  2024-03-06 17:04     ` Frieder Schrempf
  0 siblings, 1 reply; 13+ messages in thread
From: Alexey Romanov @ 2024-03-01 13:42 UTC (permalink / raw)
  To: Frieder Schrempf
  Cc: dario.binacchi, michael, xypron.glpk, ilias.apalodimas, sjg,
	bmeng, tobias, jpewhacker, marek.vasut+renesas, kever.yang,
	jbx6244, abdellatif.elkhlifi, michal.simek, mikhail.kshevetskiy,
	patrice.chotard, kernel, u-boot

Hi Frieder,

On Thu, Feb 29, 2024 at 09:51:04AM +0100, Frieder Schrempf wrote:
> Hi Alexey,
> 
> On 27.02.24 11:04, Alexey Romanov wrote:
> > Hello!
> > 
> > This series adds support for the mtdblock device, which
> > allows to read/write data block by block. For example,
> > it can now be used for BCB or Android AB command:
> > 
> >   $ bcb load mtd 0 part_name
> > 
> > Tested only on SPI NAND, so bind is made only for
> > SPI NAND drivers.
> 
> I don't know much about Android, but as far as I understand you are
> trying to store dynamic, boot-related information on the MTD device.
> 
> This might be acceptable if the underlying device is a NOR flash, but
> for any type of NAND device it sounds like a rather bad idea.
> 
> NAND devices need some kind of FTL (flash translation layer) in order to
> work reliably. Using a block filesystem directly on the NAND MTD device
> will cause problems as soon as any bad blocks or bit flips occur.
> 
> People are therefore discouraged to use mtdblock on NAND and in the
> kernel there is even a warning if you try to mount a NAND mtdblock
> partition.

You are completely right. But, unfortunately, these are legacy ones that
need to be supported. On some boards this approach was chosen a long
time ago and mistakenly, so I think we need to support this for NAND.

> 
> Maybe you should reconsider this and look into how to use UBIFS as a
> FTL. On the other hand I'm not quite sure if the UBIFS layer in U-Boot
> is really stable and maintained. So this might not be a good idea either.

Yeah, on our new boards we switched to using UBI, and we already have 
an implementation UBI block device for U-Boot. Will send this to
mailing list in the near future.

> 
> Anyway, I'm against implementing mtdblock for SPI NAND (or any other NAND).

Maybe we'll just add warning like it's done in the Linux?

> 
> Best regards
> Frieder

-- 
Thank you,
Alexey

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 0/4] Introduce mtdblock device
  2024-03-01 13:42   ` Alexey Romanov
@ 2024-03-06 17:04     ` Frieder Schrempf
  0 siblings, 0 replies; 13+ messages in thread
From: Frieder Schrempf @ 2024-03-06 17:04 UTC (permalink / raw)
  To: Alexey Romanov
  Cc: dario.binacchi, michael, xypron.glpk, ilias.apalodimas, sjg,
	bmeng, tobias, jpewhacker, marek.vasut+renesas, kever.yang,
	jbx6244, abdellatif.elkhlifi, michal.simek, mikhail.kshevetskiy,
	patrice.chotard, kernel, u-boot

Hi Alexey,

On 01.03.24 14:42, Alexey Romanov wrote:
> Hi Frieder,
> 
> On Thu, Feb 29, 2024 at 09:51:04AM +0100, Frieder Schrempf wrote:
>> Hi Alexey,
>>
>> On 27.02.24 11:04, Alexey Romanov wrote:
>>> Hello!
>>>
>>> This series adds support for the mtdblock device, which
>>> allows to read/write data block by block. For example,
>>> it can now be used for BCB or Android AB command:
>>>
>>>   $ bcb load mtd 0 part_name
>>>
>>> Tested only on SPI NAND, so bind is made only for
>>> SPI NAND drivers.
>>
>> I don't know much about Android, but as far as I understand you are
>> trying to store dynamic, boot-related information on the MTD device.
>>
>> This might be acceptable if the underlying device is a NOR flash, but
>> for any type of NAND device it sounds like a rather bad idea.
>>
>> NAND devices need some kind of FTL (flash translation layer) in order to
>> work reliably. Using a block filesystem directly on the NAND MTD device
>> will cause problems as soon as any bad blocks or bit flips occur.
>>
>> People are therefore discouraged to use mtdblock on NAND and in the
>> kernel there is even a warning if you try to mount a NAND mtdblock
>> partition.
> 
> You are completely right. But, unfortunately, these are legacy ones that
> need to be supported. On some boards this approach was chosen a long
> time ago and mistakenly, so I think we need to support this for NAND.
> 
>>
>> Maybe you should reconsider this and look into how to use UBIFS as a
>> FTL. On the other hand I'm not quite sure if the UBIFS layer in U-Boot
>> is really stable and maintained. So this might not be a good idea either.
> 
> Yeah, on our new boards we switched to using UBI, and we already have 
> an implementation UBI block device for U-Boot. Will send this to
> mailing list in the near future.
> 
>>
>> Anyway, I'm against implementing mtdblock for SPI NAND (or any other NAND).
> 
> Maybe we'll just add warning like it's done in the Linux?

If we really need this, then yes, please add a warning that is printed
to the console.

Thanks
Frieder

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/4] drivers: introduce mtdblock abstraction
  2024-02-27 13:37   ` Heinrich Schuchardt
@ 2024-03-07 10:24     ` Alexey Romanov
  0 siblings, 0 replies; 13+ messages in thread
From: Alexey Romanov @ 2024-03-07 10:24 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: kernel, u-boot, dario.binacchi, michael, frieder.schrempf,
	ilias.apalodimas, sjg, bmeng, tobias, jpewhacker,
	marek.vasut+renesas, kever.yang, jbx6244, abdellatif.elkhlifi,
	michal.simek, mikhail.kshevetskiy, patrice.chotard

Hi Heinrich,

On Tue, Feb 27, 2024 at 02:37:17PM +0100, Heinrich Schuchardt wrote:
> On 27.02.24 11:04, Alexey Romanov wrote:
> > MTD block - abstraction over MTD subsystem, allowing
> > to read and write in blocks using BLK UCLASS.
> > 
> > - Read algorithm:
> > 
> >    1. Convert start block number to start address.
> >    2. Read block_dev->blksz bytes using mtd_read() and
> >       add to start address pointer block_dev->blksz bytes,
> >       until the requested number of blocks have been read.
> > 
> > - Write algorithm:
> > 
> >    1. Convert start block number to start address.
> >    2. Round this address down by mtd->erasesize.
> > 
> >    Erase addr      Start addr
> >       |                |
> >       v                v
> >       +----------------+----------------+----------------+
> >       |     blksz      |      blksz     |      blksz     |
> >       +----------------+----------------+----------------+
> > 
> >    3. Calculate offset between this two addresses.
> >    4. Read mtd->erasesize bytes using mtd_read() into
> >       temporary buffer from erase address.
> > 
> >    Erase addr      Start addr
> >       |                |
> >       v                v
> >       +----------------+----------------+----------------+
> >       |     blksz      |      blksz     |      blksz     |
> >       +----------------+----------------+----------------+
> >       ^            mtd->erasesize = blksz * 3
> 
> Can you really erase only the start of the erase block?

Yeah.

> 
> Wouldn't you have also to round the end address up to the end of an
> erase block and read and rewrite the data between the end address and
> the end of the last erase block?

I think I didn't express myself quite correctly. Of coure, we erase
and write only by mtd->erasesize bytes and don't overwrite this field.

> >       ^            mtd->erasesize = blksz * 3

I just wrote that for example mtd->erasesize can be equal to blksz * 3,
for the situtation I describe above. In reality it can be anything. I
will describe this point in more detail in v2.


> 
> >       |
> >       |
> >       |
> > 
> >    mtd_read()
> >    from here
> > 
> >    5. Copy data from user buffer to temporary buffer with offset,
> >       calculated at step 3.
> >    6. Erase and write mtd->erasesize bytes at erase address
> >       pointer using mtd_erase/mtd_write().
> >    7. Add to erase address pointer mtd->erasesize bytes.
> >    8. goto 1 until the requested number of blocks have
> >       been written.
> 
> Commit messages is not were developers typically look for documentation.
> Please, add the information to /doc/develop/ or to the source code.

Move it to docs in v2.

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> > ---
> >   drivers/block/blk-uclass.c |   1 +
> >   drivers/mtd/Makefile       |   1 +
> >   drivers/mtd/mtdblock.c     | 170 +++++++++++++++++++++++++++++++++++++
> >   include/linux/mtd/mtd.h    |  12 +++
> >   4 files changed, 184 insertions(+)
> >   create mode 100644 drivers/mtd/mtdblock.c
> > 
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index 77066da352..ab0a9105c9 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -37,6 +37,7 @@ static struct {
> >   	{ UCLASS_PVBLOCK, "pvblock" },
> >   	{ UCLASS_BLKMAP, "blkmap" },
> >   	{ UCLASS_RKMTD, "rkmtd" },
> > +	{ UCLASS_MTD, "mtd" },
> >   };
> > 
> >   static enum uclass_id uclass_name_to_iftype(const char *uclass_idname)
> > diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> > index c638980ea2..993b122ac4 100644
> > --- a/drivers/mtd/Makefile
> > +++ b/drivers/mtd/Makefile
> > @@ -26,6 +26,7 @@ obj-y += onenand/
> >   obj-y += spi/
> >   obj-$(CONFIG_MTD_UBI) += ubi/
> >   obj-$(CONFIG_NVMXIP) += nvmxip/
> > +obj-$(CONFIG_BLK) += mtdblock.o
> > 
> >   #SPL/TPL build
> >   else
> > diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
> > new file mode 100644
> > index 0000000000..0563d0b795
> > --- /dev/null
> > +++ b/drivers/mtd/mtdblock.c
> > @@ -0,0 +1,170 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2024 SaluteDevices, Inc.
> > + *
> > + * Author: Alexey Romanov <avromanov@salutedevices.com>
> > + */
> > +
> > +#include <blk.h>
> > +#include <dm/device.h>
> > +#include <dm/device-internal.h>
> > +#include <linux/mtd/mtd.h>
> > +
> > +int mtd_bind(struct udevice *dev, struct mtd_info **mtd)
> > +{
> > +	struct blk_desc *bdesc;
> > +	struct udevice *bdev;
> > +	int ret;
> > +
> > +	ret = blk_create_devicef(dev, "mtd_blk", "blk", UCLASS_MTD,
> > +				 dev_seq(dev), 512, 0, &bdev);
> > +	if (ret) {
> > +		pr_err("Cannot create block device");
> > +		return ret;
> > +	}
> > +
> > +	bdesc = dev_get_uclass_plat(bdev);
> > +	dev_set_priv(bdev, mtd);
> > +	bdesc->bdev = bdev;
> > +
> > +	return 0;
> > +}
> > +
> > +static ulong mtd_bread(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> > +		       void *dst)
> > +{
> > +	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
> > +	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
> > +	unsigned int sect_size = block_dev->blksz;
> > +	lbaint_t cur = start;
> > +	ulong read_cnt = 0;
> > +
> > +	while (read_cnt < blkcnt) {
> > +		int ret;
> > +		loff_t sect_start = cur * sect_size;
> > +		size_t retlen;
> > +
> > +		ret = mtd_read(mtd, sect_start, sect_size, &retlen, dst);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (retlen != sect_size) {
> > +			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
> > +			return -EIO;
> > +		}
> > +
> > +		cur++;
> > +		dst += sect_size;
> > +		read_cnt++;
> > +	}
> > +
> > +	return read_cnt;
> > +}
> > +
> > +static int mtd_erase_write(struct mtd_info *mtd, uint64_t start, const void *src)
> > +{
> > +	int ret;
> > +	size_t retlen;
> > +	struct erase_info erase = { 0 };
> > +
> > +	erase.mtd = mtd;
> > +	erase.addr = start;
> > +	erase.len = mtd->erasesize;
> > +
> > +	ret = mtd_erase(mtd, &erase);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtd_write(mtd, start, mtd->erasesize, &retlen, src);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (retlen != mtd->erasesize) {
> > +		pr_err("mtdblock: failed to read block at 0x%llx\n", start);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static ulong mtd_bwrite(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> > +			const void *src)
> > +{
> > +	struct blk_desc *block_dev = dev_get_uclass_plat(dev);
> > +	struct mtd_info *mtd = blk_desc_to_mtd(block_dev);
> > +	unsigned int sect_size = block_dev->blksz;
> > +	lbaint_t cur = start, blocks_todo = blkcnt;
> > +	ulong write_cnt = 0;
> > +	u8 *buf;
> > +	int ret = 0;
> > +
> > +	buf = malloc(mtd->erasesize);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	while (blocks_todo > 0) {
> > +		loff_t sect_start = cur * sect_size;
> > +		loff_t erase_start = ALIGN_DOWN(sect_start, mtd->erasesize);
> > +		u32 offset = sect_start - erase_start;
> > +		size_t cur_size = min_t(size_t,  mtd->erasesize - offset,
> > +					blocks_todo * sect_size);
> > +		size_t retlen;
> > +		lbaint_t written;
> > +
> > +		ret = mtd_read(mtd, erase_start, mtd->erasesize, &retlen, buf);
> > +		if (ret)
> > +			goto out;
> > +
> > +		if (retlen != mtd->erasesize) {
> > +			pr_err("mtdblock: failed to read block 0x%lx\n", cur);
> > +			ret = -EIO;
> > +			goto out;
> > +		}
> > +
> > +		memcpy(buf + offset, src, cur_size);
> > +
> > +		ret = mtd_erase_write(mtd, erase_start, buf);
> > +		if (ret)
> > +			goto out;
> > +
> > +		written = cur_size / sect_size;
> > +
> > +		blocks_todo -= written;
> > +		cur += written;
> > +		src += cur_size;
> > +		write_cnt += written;
> > +	}
> > +
> > +out:
> > +	free(buf);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return write_cnt;
> > +}
> > +
> > +static int mtd_blk_probe(struct udevice *dev)
> > +{
> > +	int ret;
> > +
> > +	ret = device_probe(dev);
> > +	if (ret) {
> > +		pr_err("Probing %s failed (err=%d)\n", dev->name, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct blk_ops mtd_blk_ops = {
> > +	.read = mtd_bread,
> > +	.write = mtd_bwrite,
> > +};
> > +
> > +U_BOOT_DRIVER(mtd_blk) = {
> > +	.name = "mtd_blk",
> > +	.id = UCLASS_BLK,
> > +	.ops = &mtd_blk_ops,
> > +	.probe = mtd_blk_probe,
> > +};
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 09f5269887..9b997fadd1 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -26,6 +26,9 @@
> >   #include <dm/device.h>
> >   #endif
> >   #include <dm/ofnode.h>
> > +#if IS_ENABLED(CONFIG_BLK)
> > +#include <blk.h>
> > +#endif
> > 
> >   #define MAX_MTD_DEVICES 32
> >   #endif
> > @@ -412,6 +415,15 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
> >   int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
> >   		    const u_char *buf);
> > 
> > +#if IS_ENABLED(CONFIG_BLK)
> > +static inline struct mtd_info *blk_desc_to_mtd(struct blk_desc *bdesc)
> > +{
> > +	return *((struct mtd_info **)dev_get_priv(bdesc->bdev));
> > +}
> > +
> > +int mtd_bind(struct udevice *dev, struct mtd_info **mtd);
> > +#endif
> > +
> >   int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
> >   int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
> > 
> 

-- 
Thank you,
Alexey

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-03-07 14:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-27 10:04 [PATCH v1 0/4] Introduce mtdblock device Alexey Romanov
2024-02-27 10:04 ` [PATCH v1 1/4] drivers: introduce mtdblock abstraction Alexey Romanov
2024-02-27 13:37   ` Heinrich Schuchardt
2024-03-07 10:24     ` Alexey Romanov
2024-02-27 10:04 ` [PATCH v1 2/4] disk: support MTD partitions Alexey Romanov
2024-02-27 13:28   ` Heinrich Schuchardt
2024-02-27 10:04 ` [PATCH v1 3/4] spinand: bind mtdblock Alexey Romanov
2024-02-27 13:23   ` Heinrich Schuchardt
2024-02-27 10:04 ` [PATCH v1 4/4] efi: block: compile only if CONFIG_EFI_PARTITION enabled Alexey Romanov
2024-02-27 13:18   ` Heinrich Schuchardt
2024-02-29  8:51 ` [PATCH v1 0/4] Introduce mtdblock device Frieder Schrempf
2024-03-01 13:42   ` Alexey Romanov
2024-03-06 17:04     ` Frieder Schrempf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.