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);

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

*/

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)

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