[PATCH v4 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[PATCH v4 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Introduce support for mtd backed storage for FWU feature and enable it on
Synquacer platform based DeveloperBox.

This revision is rebased onto patchset that trims the FWU api
 https://lore.kernel.org/u-boot/20230306231747.1888513-1-jassisinghbrar@gmail.com/

Changes since v3:
	* Fix and Update documentation to also build optee for FWU FIP image.
	* Fixed checkpatch warnings
	* Made local functions static.
	* Split config changes to a separate patch
	* Fix authorship of three patches.


Jassi Brar (3):
  dt: fwu: developerbox: enable fwu banks and mdata regions
  configs: move to new flash layout and boot flow
  fwu: DeveloperBox: add support for FWU

Masami Hiramatsu (3):
  FWU: Add FWU metadata access driver for MTD storage regions
  FWU: mtd: Add helper functions for accessing FWU metadata
  tools: Add mkfwumdata tool for FWU metadata image

 .../synquacer-sc2a11-developerbox-u-boot.dtsi |  49 ++-
 board/socionext/developerbox/Makefile         |   1 +
 board/socionext/developerbox/developerbox.c   |   8 +
 board/socionext/developerbox/fwu_plat.c       |  57 +++
 configs/synquacer_developerbox_defconfig      |  12 +-
 doc/board/socionext/developerbox.rst          | 155 +++++++-
 drivers/fwu-mdata/Kconfig                     |  15 +
 drivers/fwu-mdata/Makefile                    |   1 +
 drivers/fwu-mdata/raw_mtd.c                   | 272 ++++++++++++++
 include/configs/synquacer.h                   |  10 +
 include/fwu.h                                 |  34 ++
 lib/fwu_updates/Makefile                      |   1 +
 lib/fwu_updates/fwu_mtd.c                     | 164 +++++++++
 tools/Kconfig                                 |   9 +
 tools/Makefile                                |   4 +
 tools/mkfwumdata.c                            | 334 ++++++++++++++++++
 16 files changed, 1115 insertions(+), 11 deletions(-)
 create mode 100644 board/socionext/developerbox/fwu_plat.c
 create mode 100644 drivers/fwu-mdata/raw_mtd.c
 create mode 100644 lib/fwu_updates/fwu_mtd.c
 create mode 100644 tools/mkfwumdata.c

-- 
2.34.1

[PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[jassisinghbrar]

From: Masami Hiramatsu <masami.hiramatsu@linaro.org>

In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, on a separate
region. Add a driver for reading from and writing to the metadata
when the updatable images and the metadata are stored on a raw
MTD region.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/Kconfig   |  15 ++
 drivers/fwu-mdata/Makefile  |   1 +
 drivers/fwu-mdata/raw_mtd.c | 272 ++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/fwu-mdata/raw_mtd.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index 36c4479a59..42736a5e43 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -6,6 +6,11 @@ config FWU_MDATA
 	  FWU Metadata partitions reside on the same storage device
 	  which contains the other FWU updatable firmware images.
 
+choice
+	prompt "Storage Layout Scheme"
+	depends on FWU_MDATA
+	default FWU_MDATA_GPT_BLK
+
 config FWU_MDATA_GPT_BLK
 	bool "FWU Metadata access for GPT partitioned Block devices"
 	select PARTITION_TYPE_GUID
@@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
 	help
 	  Enable support for accessing FWU Metadata on GPT partitioned
 	  block devices.
+
+config FWU_MDATA_MTD
+	bool "Raw MTD devices"
+	depends on MTD
+	help
+	  Enable support for accessing FWU Metadata on non-partitioned
+	  (or non-GPT partitioned, e.g. partition nodes in devicetree)
+	  MTD devices.
+
+endchoice
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index 3fee64c10c..06c49747ba 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -6,3 +6,4 @@
 
 obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
 obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
+obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
new file mode 100644
index 0000000000..4b1a10073a
--- /dev/null
+++ b/drivers/fwu-mdata/raw_mtd.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <memalign.h>
+
+#include <linux/errno.h>
+#include <linux/types.h>
+
+/* Internal helper structure to move data around */
+struct fwu_mdata_mtd_priv {
+	struct mtd_info *mtd;
+	char pri_label[50];
+	char sec_label[50];
+	u32 pri_offset;
+	u32 sec_offset;
+};
+
+enum fwu_mtd_op {
+	FWU_MTD_READ,
+	FWU_MTD_WRITE,
+};
+
+extern struct fwu_mtd_image_info fwu_mtd_images[];
+
+static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
+{
+	return !do_div(size, mtd->erasesize);
+}
+
+static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
+		       enum fwu_mtd_op op)
+{
+	struct mtd_oob_ops io_op = {};
+	u64 lock_offs, lock_len;
+	size_t len;
+	void *buf;
+	int ret;
+
+	if (!mtd_is_aligned_with_block_size(mtd, offs)) {
+		log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
+		return -EINVAL;
+	}
+
+	lock_offs = offs;
+	/* This will expand erase size to align with the block size */
+	lock_len = round_up(size, mtd->erasesize);
+
+	ret = mtd_unlock(mtd, lock_offs, lock_len);
+	if (ret && ret != -EOPNOTSUPP)
+		return ret;
+
+	if (op == FWU_MTD_WRITE) {
+		struct erase_info erase_op = {};
+
+		erase_op.mtd = mtd;
+		erase_op.addr = lock_offs;
+		erase_op.len = lock_len;
+		erase_op.scrub = 0;
+
+		ret = mtd_erase(mtd, &erase_op);
+		if (ret)
+			goto lock;
+	}
+
+	/* Also, expand the write size to align with the write size */
+	len = round_up(size, mtd->writesize);
+
+	buf = memalign(ARCH_DMA_MINALIGN, len);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto lock;
+	}
+	memset(buf, 0xff, len);
+
+	io_op.mode = MTD_OPS_AUTO_OOB;
+	io_op.len = len;
+	io_op.ooblen = 0;
+	io_op.datbuf = buf;
+	io_op.oobbuf = NULL;
+
+	if (op == FWU_MTD_WRITE) {
+		memcpy(buf, data, size);
+		ret = mtd_write_oob(mtd, offs, &io_op);
+	} else {
+		ret = mtd_read_oob(mtd, offs, &io_op);
+		if (!ret)
+			memcpy(data, buf, size);
+	}
+	free(buf);
+
+lock:
+	mtd_lock(mtd, lock_offs, lock_len);
+
+	return ret;
+}
+
+static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	struct mtd_info *mtd = mtd_priv->mtd;
+	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
+
+	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
+}
+
+static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	struct mtd_info *mtd = mtd_priv->mtd;
+	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
+
+	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
+}
+
+static int flash_partition_offset(struct udevice *dev, const char *part_name, fdt_addr_t *offset)
+{
+	ofnode node, parts_node;
+	fdt_addr_t size = 0;
+
+	parts_node = ofnode_by_compatible(dev_ofnode(dev), "fixed-partitions");
+	node = ofnode_by_prop_value(parts_node, "label", part_name, strlen(part_name) + 1);
+	if (!ofnode_valid(node)) {
+		log_err("Warning: Failed to find partition by label <%s>\n", part_name);
+		return -ENOENT;
+	}
+
+	*offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
+
+	return (int)size;
+}
+
+static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	const fdt32_t *phandle_p = NULL;
+	struct udevice *mtd_dev;
+	struct mtd_info *mtd;
+	const char *label;
+	fdt_addr_t offset;
+	int ret, size;
+	u32 phandle;
+	ofnode bank;
+	int off_img;
+
+	/* Find the FWU mdata storage device */
+	phandle_p = ofnode_get_property(dev_ofnode(dev),
+					"fwu-mdata-store", &size);
+	if (!phandle_p) {
+		log_err("FWU meta data store not defined in device-tree\n");
+		return -ENOENT;
+	}
+
+	phandle = fdt32_to_cpu(*phandle_p);
+
+	ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
+					  &mtd_dev);
+	if (ret) {
+		log_err("FWU: failed to get mtd device\n");
+		return ret;
+	}
+
+	mtd_probe_devices();
+
+	mtd_for_each_device(mtd) {
+		if (mtd->dev == mtd_dev) {
+			mtd_priv->mtd = mtd;
+			log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
+			break;
+		}
+	}
+	if (!mtd_priv->mtd) {
+		log_err("Failed to find mtd device by fwu-mdata-store\n");
+		return -ENODEV;
+	}
+
+	/* Get the offset of primary and seconday mdata */
+	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0, &label);
+	if (ret)
+		return ret;
+	strcpy(mtd_priv->pri_label, label);
+
+	ret = flash_partition_offset(mtd_dev, mtd_priv->pri_label, &offset);
+	if (ret <= 0)
+		return ret;
+	mtd_priv->pri_offset = offset;
+
+	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 1, &label);
+	if (ret)
+		return ret;
+	strcpy(mtd_priv->sec_label, label);
+
+	ret = flash_partition_offset(mtd_dev, mtd_priv->sec_label, &offset);
+	if (ret <= 0)
+		return ret;
+	mtd_priv->sec_offset = offset;
+
+	off_img = 0;
+
+	ofnode_for_each_subnode(bank, dev_ofnode(dev)) {
+		int bank_num, bank_offset, bank_size;
+		const char *bank_name;
+		ofnode image;
+
+		ofnode_read_u32(bank, "id", &bank_num);
+		bank_name = ofnode_read_string(bank, "label");
+		bank_size = flash_partition_offset(mtd_dev, bank_name, &offset);
+		if (bank_size <= 0)
+			return bank_size;
+		bank_offset = offset;
+		log_debug("Bank%d: %s [0x%x - 0x%x]\n",
+			  bank_num, bank_name, bank_offset, bank_offset + bank_size);
+
+		ofnode_for_each_subnode(image, bank) {
+			int image_num, image_offset, image_size;
+			const char *uuid;
+
+			if (off_img == CONFIG_FWU_NUM_BANKS *
+						CONFIG_FWU_NUM_IMAGES_PER_BANK) {
+				log_err("DT provides images more than configured!\n");
+				break;
+			}
+
+			uuid = ofnode_read_string(image, "uuid");
+			ofnode_read_u32(image, "id", &image_num);
+			ofnode_read_u32(image, "offset", &image_offset);
+			ofnode_read_u32(image, "size", &image_size);
+
+			fwu_mtd_images[off_img].start = bank_offset + image_offset;
+			fwu_mtd_images[off_img].size = image_size;
+			fwu_mtd_images[off_img].bank_num = bank_num;
+			fwu_mtd_images[off_img].image_num = image_num;
+			strcpy(fwu_mtd_images[off_img].uuidbuf, uuid);
+			log_debug("\tImage%d: %s @0x%x\n\n",
+				  image_num, uuid, bank_offset + image_offset);
+			off_img++;
+		}
+	}
+
+	return 0;
+}
+
+static int fwu_mdata_mtd_probe(struct udevice *dev)
+{
+	/* Ensure the metadata can be read. */
+	return fwu_get_mdata(NULL);
+}
+
+static struct fwu_mdata_ops fwu_mtd_ops = {
+	.read_mdata = fwu_mtd_read_mdata,
+	.write_mdata = fwu_mtd_write_mdata,
+};
+
+static const struct udevice_id fwu_mdata_ids[] = {
+	{ .compatible = "u-boot,fwu-mdata-mtd" },
+	{ }
+};
+
+U_BOOT_DRIVER(fwu_mdata_mtd) = {
+	.name		= "fwu-mdata-mtd",
+	.id		= UCLASS_FWU_MDATA,
+	.of_match	= fwu_mdata_ids,
+	.ops		= &fwu_mtd_ops,
+	.probe		= fwu_mdata_mtd_probe,
+	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
+	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
+};
-- 
2.34.1

[PATCH v4 2/6] FWU: mtd: Add helper functions for accessing FWU metadata

[jassisinghbrar]

From: Masami Hiramatsu <masami.hiramatsu@linaro.org>

Add helper functions needed for accessing the FWU metadata which
contains information on the updatable images.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 include/fwu.h             |  34 ++++++++
 lib/fwu_updates/Makefile  |   1 +
 lib/fwu_updates/fwu_mtd.c | 164 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)
 create mode 100644 lib/fwu_updates/fwu_mtd.c

diff --git a/include/fwu.h b/include/fwu.h
index 33b4d0b3db..117f51c4f5 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -8,6 +8,8 @@
 
 #include <blk.h>
 #include <efi.h>
+#include <mtd.h>
+#include <uuid.h>
 
 #include <linux/types.h>
 
@@ -18,6 +20,12 @@ struct fwu_mdata_gpt_blk_priv {
 	struct udevice *blk_dev;
 };
 
+struct fwu_mtd_image_info {
+	u32 start, size;
+	int bank_num, image_num;
+	char uuidbuf[UUID_STR_LEN + 1];
+};
+
 struct fwu_mdata_ops {
 	/**
 	 * read_mdata() - Populate the asked FWU metadata copy
@@ -251,4 +259,30 @@ u8 fwu_empty_capsule_checks_pass(void);
  */
 int fwu_trial_state_ctr_start(void);
 
+/**
+ * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
+ * @buf: Buffer into which the dfu_alt_info is filled
+ * @len: Maximum characters that can be written in buf
+ * @mtd: Pointer to underlying MTD device
+ *
+ * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
+
+/**
+ * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
+ * @image_guid: Image GUID for which DFU alt number needs to be retrieved
+ * @alt_num: Pointer to the alt_num
+ * @mtd_dev: Name of mtd device instance
+ *
+ * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char *mtd_dev);
+
 #endif /* _FWU_H_ */
diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
index 1993088e5b..c9e3c06b48 100644
--- a/lib/fwu_updates/Makefile
+++ b/lib/fwu_updates/Makefile
@@ -5,3 +5,4 @@
 
 obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
 obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
+obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mtd.o
diff --git a/lib/fwu_updates/fwu_mtd.c b/lib/fwu_updates/fwu_mtd.c
new file mode 100644
index 0000000000..c1d04e574b
--- /dev/null
+++ b/lib/fwu_updates/fwu_mtd.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <dm.h>
+#include <dfu.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <log.h>
+#include <malloc.h>
+#include <mtd.h>
+#include <uuid.h>
+#include <vsprintf.h>
+
+#include <dm/ofnode.h>
+
+struct fwu_mtd_image_info
+fwu_mtd_images[CONFIG_FWU_NUM_BANKS * CONFIG_FWU_NUM_IMAGES_PER_BANK];
+
+static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
+{
+	int num_images = sizeof(fwu_mtd_images) / sizeof(struct fwu_mtd_image_info);
+
+	for (int i = 0; i < num_images; i++)
+		if (!strcmp(uuidbuf, fwu_mtd_images[i].uuidbuf))
+			return &fwu_mtd_images[i];
+
+	return NULL;
+}
+
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
+			const char *mtd_dev)
+{
+	int i, nalt;
+	int ret = -1;
+	struct mtd_info *mtd;
+	struct dfu_entity *dfu;
+	fdt_addr_t offset, size = 0;
+	char uuidbuf[UUID_STR_LEN + 1];
+	struct fwu_mtd_image_info *mtd_img_info;
+
+	mtd_probe_devices();
+	mtd = get_mtd_device_nm(mtd_dev);
+
+	uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
+
+	mtd_img_info = mtd_img_by_uuid(uuidbuf);
+	if (!mtd_img_info) {
+		log_err("%s: Not found partition for image %s\n", __func__, uuidbuf);
+		return -ENOENT;
+	}
+	offset = mtd_img_info->start;
+	size = mtd_img_info->size;
+
+	dfu_init_env_entities(NULL, NULL);
+
+	nalt = 0;
+	list_for_each_entry(dfu, &dfu_list, list)
+		nalt++;
+
+	if (!nalt) {
+		log_warning("No entities in dfu_alt_info\n");
+		dfu_free_entities();
+		return -ENOENT;
+	}
+
+	for (i = 0; i < nalt; i++) {
+		dfu = dfu_get_entity(i);
+
+		/* Only MTD RAW access */
+		if (!dfu || dfu->dev_type != DFU_DEV_MTD ||
+		    dfu->layout != DFU_RAW_ADDR ||
+			dfu->data.mtd.start != offset ||
+			dfu->data.mtd.size != size)
+			continue;
+
+		*alt_num = dfu->alt;
+		ret = 0;
+		break;
+	}
+
+	dfu_free_entities();
+
+	log_debug("%s: %s -> %d\n", __func__, uuidbuf, *alt_num);
+	return ret;
+}
+
+static int gen_image_alt_info(char *buf, size_t len, int sidx,
+			      struct fwu_image_entry *img, struct mtd_info *mtd)
+{
+	int i;
+	char *p = buf, *end = buf + len;
+
+	p += snprintf(p, end - p, "mtd %s", mtd->name);
+	if (end < p) {
+		log_err("%s:%d Run out of buffer\n", __func__, __LINE__);
+		return -E2BIG;
+	}
+
+	/*
+	 * List the image banks in the FWU mdata and search the corresponding
+	 * partition based on partition's uuid.
+	 */
+	for (i = 0; i < CONFIG_FWU_NUM_BANKS; i++) {
+		struct fwu_mtd_image_info *mtd_img_info;
+		struct fwu_image_bank_info *bank;
+		char uuidbuf[UUID_STR_LEN + 1];
+		u32 offset, size;
+
+		/* Query a partition by image UUID */
+		bank = &img->img_bank_info[i];
+		uuid_bin_to_str(bank->image_uuid.b, uuidbuf, UUID_STR_FORMAT_STD);
+
+		mtd_img_info = mtd_img_by_uuid(uuidbuf);
+		if (!mtd_img_info) {
+			log_err("%s: Not found partition for image %s\n", __func__, uuidbuf);
+			break;
+		}
+
+		offset = mtd_img_info->start;
+		size = mtd_img_info->size;
+
+		p += snprintf(p, end - p, "%sbank%d raw %x %x",
+			      i == 0 ? "=" : ";", i, offset, size);
+		if (end < p) {
+			log_err("%s:%d Run out of buffer\n", __func__, __LINE__);
+			return -E2BIG;
+		}
+	}
+
+	if (i == CONFIG_FWU_NUM_BANKS)
+		return 0;
+
+	return -ENOENT;
+}
+
+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd)
+{
+	struct fwu_mdata mdata;
+	int i, l, ret;
+
+	ret = fwu_get_mdata(&mdata);
+	if (ret < 0) {
+		log_err("Failed to get the FWU mdata.\n");
+		return ret;
+	}
+
+	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
+		ret = gen_image_alt_info(buf, len, i * CONFIG_FWU_NUM_BANKS,
+					 &mdata.img_entry[i], mtd);
+		if (ret)
+			break;
+
+		l = strlen(buf);
+		/* Replace the last ';' with '&' if there is another image. */
+		if (i != CONFIG_FWU_NUM_IMAGES_PER_BANK - 1 && l)
+			buf[l - 1] = '&';
+		len -= l;
+		buf += l;
+	}
+
+	return ret;
+}
-- 
2.34.1

[PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[jassisinghbrar]

From: Masami Hiramatsu <masami.hiramatsu@linaro.org>

Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
partition to be used in A/B Update imeplementation.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 tools/Kconfig      |   9 ++
 tools/Makefile     |   4 +
 tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 tools/mkfwumdata.c

diff --git a/tools/Kconfig b/tools/Kconfig
index 539708f277..6e23f44d55 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -157,4 +157,13 @@ config LUT_SEQUENCE
 	help
 	  Look Up Table Sequence
 
+config TOOLS_MKFWUMDATA
+	bool "Build mkfwumdata command"
+	default y if FWU_MULTI_BANK_UPDATE
+	help
+	  This command allows users to create a raw image of the FWU
+	  metadata for initial installation of the FWU multi bank
+	  update on the board. The installation method depends on
+	  the platform.
+
 endmenu
diff --git a/tools/Makefile b/tools/Makefile
index e13effbb66..80eee71505 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -247,6 +247,10 @@ HOSTLDLIBS_mkeficapsule += \
 	$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
 hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
 
+mkfwumdata-objs := mkfwumdata.o lib/crc32.o
+HOSTLDLIBS_mkfwumdata += -luuid
+hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
+
 # We build some files with extra pedantic flags to try to minimize things
 # that won't build on some weird host compiler -- though there are lots of
 # exceptions for files that aren't complaint.
diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
new file mode 100644
index 0000000000..43dabf3b72
--- /dev/null
+++ b/tools/mkfwumdata.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <errno.h>
+#include <getopt.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <u-boot/crc.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+/* This will dynamically allocate the fwu_mdata */
+#define CONFIG_FWU_NUM_BANKS		0
+#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0
+
+/* Since we can not include fwu.h, redefine version here. */
+#define FWU_MDATA_VERSION		1
+
+typedef uint8_t u8;
+typedef int16_t s16;
+typedef uint16_t u16;
+typedef uint32_t u32;
+typedef uint64_t u64;
+
+#include <fwu_mdata.h>
+
+/* TODO: Endianness conversion may be required for some arch. */
+
+static const char *opts_short = "b:i:a:p:gh";
+
+static struct option options[] = {
+	{"banks", required_argument, NULL, 'b'},
+	{"images", required_argument, NULL, 'i'},
+	{"guid", required_argument, NULL, 'g'},
+	{"active-bank", required_argument, NULL, 'a'},
+	{"previous-bank", required_argument, NULL, 'p'},
+	{"help", no_argument, NULL, 'h'},
+	{NULL, 0, NULL, 0},
+};
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\n");
+	fprintf(stderr, "Options:\n"
+		"\t-i, --images <num>          Number of images\n"
+		"\t-b, --banks  <num>          Number of banks\n"
+		"\t-a, --active-bank  <num>    Active bank\n"
+		"\t-p, --previous-bank  <num>  Previous active bank\n"
+		"\t-g, --guid                  Use GUID instead of UUID\n"
+		"\t-h, --help                  print a help message\n"
+		);
+	fprintf(stderr, "  UUIDs list syntax:\n"
+		"\t  <location uuid>,<image type uuid>,<images uuid list>\n"
+		"\t     images uuid list syntax:\n"
+		"\t        img_uuid_00,img_uuid_01...img_uuid_0b,\n"
+		"\t        img_uuid_10,img_uuid_11...img_uuid_1b,\n"
+		"\t        ...,\n"
+		"\t        img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
+		"\t          where 'b' and 'i' are number of banks and number\n"
+		"\t          of images in a bank respectively.\n"
+	       );
+}
+
+struct fwu_mdata_object {
+	size_t images;
+	size_t banks;
+	size_t size;
+	struct fwu_mdata *mdata;
+};
+
+static int previous_bank, active_bank;
+static bool __use_guid;
+
+static struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
+{
+	struct fwu_mdata_object *mobj;
+
+	mobj = calloc(1, sizeof(*mobj));
+	if (!mobj)
+		return NULL;
+
+	mobj->size = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * banks) * images;
+	mobj->images = images;
+	mobj->banks = banks;
+
+	mobj->mdata = calloc(1, mobj->size);
+	if (!mobj->mdata) {
+		free(mobj);
+		return NULL;
+	}
+
+	return mobj;
+}
+
+static struct fwu_image_entry *
+fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)
+{
+	size_t offset;
+
+	offset = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
+
+	return (struct fwu_image_entry *)((char *)mobj->mdata + offset);
+}
+
+static struct fwu_image_bank_info *
+fwu_get_bank(struct fwu_mdata_object *mobj, size_t img_idx, size_t bnk_idx)
+{
+	size_t offset;
+
+	offset = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
+		sizeof(struct fwu_image_entry) +
+		sizeof(struct fwu_image_bank_info) * bnk_idx;
+
+	return (struct fwu_image_bank_info *)((char *)mobj->mdata + offset);
+}
+
+/**
+ * convert_uuid_to_guid() - convert UUID to GUID
+ * @buf:	UUID binary
+ *
+ * UUID and GUID have the same data structure, but their binary
+ * formats are different due to the endianness. See lib/uuid.c.
+ * Since uuid_parse() can handle only UUID, this function must
+ * be called to get correct data for GUID when parsing a string.
+ *
+ * The correct data will be returned in @buf.
+ */
+static void convert_uuid_to_guid(unsigned char *buf)
+{
+	unsigned char c;
+
+	c = buf[0];
+	buf[0] = buf[3];
+	buf[3] = c;
+	c = buf[1];
+	buf[1] = buf[2];
+	buf[2] = c;
+
+	c = buf[4];
+	buf[4] = buf[5];
+	buf[5] = c;
+
+	c = buf[6];
+	buf[6] = buf[7];
+	buf[7] = c;
+}
+
+static int uuid_guid_parse(char *uuidstr, unsigned char *uuid)
+{
+	int ret;
+
+	ret = uuid_parse(uuidstr, uuid);
+	if (ret < 0)
+		return ret;
+
+	if (__use_guid)
+		convert_uuid_to_guid(uuid);
+
+	return ret;
+}
+
+static int
+fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
+			  size_t idx, char *uuids)
+{
+	struct fwu_image_entry *image = fwu_get_image(mobj, idx);
+	struct fwu_image_bank_info *bank;
+	char *p = uuids, *uuid;
+	int i;
+
+	if (!image)
+		return -ENOENT;
+
+	/* Image location UUID */
+	uuid = strsep(&p, ",");
+	if (!uuid)
+		return -EINVAL;
+
+	if (strcmp(uuid, "0") &&
+	    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
+		return -EINVAL;
+
+	/* Image type UUID */
+	uuid = strsep(&p, ",");
+	if (!uuid)
+		return -EINVAL;
+
+	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)
+		return -EINVAL;
+
+	/* Fill bank image-UUID */
+	for (i = 0; i < mobj->banks; i++) {
+		bank = fwu_get_bank(mobj, idx, i);
+		if (!bank)
+			return -ENOENT;
+		bank->accepted = 1;
+		uuid = strsep(&p, ",");
+		if (!uuid)
+			return -EINVAL;
+
+		if (strcmp(uuid, "0") &&
+		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
+			return -EINVAL;
+	}
+	return 0;
+}
+
+/* Caller must ensure that @uuids[] has @mobj->images entries. */
+static int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[])
+{
+	struct fwu_mdata *mdata = mobj->mdata;
+	int i, ret;
+
+	mdata->version = FWU_MDATA_VERSION;
+	mdata->active_index = active_bank;
+	mdata->previous_active_index = previous_bank;
+
+	for (i = 0; i < mobj->images; i++) {
+		ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,
+			     mobj->size - sizeof(uint32_t));
+
+	return 0;
+}
+
+static int
+fwu_make_mdata(size_t images, size_t banks, char *uuids[], char *output)
+{
+	struct fwu_mdata_object *mobj;
+	FILE *file;
+	int ret;
+
+	mobj = fwu_alloc_mdata(images, banks);
+	if (!mobj)
+		return -ENOMEM;
+
+	ret = fwu_parse_fill_uuids(mobj, uuids);
+	if (ret < 0)
+		goto done_make;
+
+	file = fopen(output, "w");
+	if (!file) {
+		ret = -errno;
+		goto done_make;
+	}
+
+	ret = fwrite(mobj->mdata, mobj->size, 1, file);
+	if (ret != mobj->size)
+		ret = -errno;
+	else
+		ret = 0;
+
+	fclose(file);
+
+done_make:
+	free(mobj->mdata);
+	free(mobj);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	unsigned long banks = 0, images = 0;
+	int c, ret;
+
+	/* Explicitly initialize defaults */
+	active_bank = 0;
+	__use_guid = false;
+	previous_bank = INT_MAX;
+
+	do {
+		c = getopt_long(argc, argv, opts_short, options, NULL);
+		switch (c) {
+		case 'h':
+			print_usage();
+			return 0;
+		case 'b':
+			banks = strtoul(optarg, NULL, 0);
+			break;
+		case 'i':
+			images = strtoul(optarg, NULL, 0);
+			break;
+		case 'g':
+			__use_guid = true;
+			break;
+		case 'p':
+			previous_bank = strtoul(optarg, NULL, 0);
+			break;
+		case 'a':
+			active_bank = strtoul(optarg, NULL, 0);
+			break;
+		}
+	} while (c != -1);
+
+	if (!banks || !images) {
+		fprintf(stderr, "Error: The number of banks and images must not be 0.\n");
+		return -EINVAL;
+	}
+
+	/* This command takes UUIDs * images and output file. */
+	if (optind + images + 1 != argc) {
+		fprintf(stderr, "Error: UUID list or output file is not specified or too much.\n");
+		print_usage();
+		return -ERANGE;
+	}
+
+	if (previous_bank == INT_MAX) {
+		/* set to the earlier bank in round-robin scheme */
+		previous_bank = active_bank > 0 ? active_bank - 1 : banks - 1;
+	}
+
+	ret = fwu_make_mdata(images, banks, argv + optind, argv[argc - 1]);
+	if (ret < 0)
+		fprintf(stderr, "Error: Failed to parse and write image: %s\n",
+			strerror(-ret));
+
+	return ret;
+}
-- 
2.34.1

[PATCH v4 4/6] dt: fwu: developerbox: enable fwu banks and mdata regions

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Specify Bank-0/1 and fwu metadata mtd regions.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 .../synquacer-sc2a11-developerbox-u-boot.dtsi | 49 +++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
index 9f9837b33b..9957646a46 100644
--- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
+++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
@@ -21,7 +21,7 @@
 		#size-cells = <0>;
 		status = "okay";
 
-		flash@0 {
+		flash0: flash@0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "jedec,spi-nor";
@@ -74,8 +74,24 @@
 				};
 
 				partition@500000 {
-					label = "Ex-OPTEE";
-					reg = <0x500000 0x200000>;
+					label = "MDATA-Pri";
+					reg = <0x500000 0x1000>;
+				};
+
+				partition@530000 {
+					label = "MDATA-Sec";
+					reg = <0x530000 0x1000>;
+				};
+
+				/* FWU Multi bank update partitions */
+				partition@600000 {
+					label = "FIP-Bank0";
+					reg = <0x600000 0x400000>;
+				};
+
+				partition@a00000 {
+					label = "FIP-Bank1";
+					reg = <0xa00000 0x400000>;
 				};
 			};
 		};
@@ -102,6 +118,33 @@
 		optee {
 			status = "okay";
 		};
+
+		fwu-mdata {
+			compatible = "u-boot,fwu-mdata-mtd";
+			fwu-mdata-store = <&flash0>;
+			mdata-parts = "MDATA-Pri", "MDATA-Sec";
+
+			fwu-bank0 {
+				id = <0>;
+				label = "FIP-Bank0";
+				fwu-image0 {
+					id = <0>;
+					offset = <0x0>;
+					size = <0x400000>;
+					uuid = "5a66a702-99fd-4fef-a392-c26e261a2828";
+				};
+			};
+			fwu-bank1 {
+				id = <1>;
+				label = "FIP-Bank1";
+				fwu-image0 {
+					id = <0>;
+					offset = <0x0>;
+					size = <0x400000>;
+					uuid = "a8f868a1-6e5c-4757-878d-ce63375ef2c0";
+				};
+			};
+		};
 	};
 };
 
-- 
2.34.1

[PATCH v4 5/6] configs: move to new flash layout and boot flow

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Towards enabling FWU and supporting new firmware layout in NOR flash,
make u-boot PIC and adjust uboot env offset in flash.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 configs/synquacer_developerbox_defconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
index 08f19a90cb..09e12b739b 100644
--- a/configs/synquacer_developerbox_defconfig
+++ b/configs/synquacer_developerbox_defconfig
@@ -1,12 +1,12 @@
 CONFIG_ARM=y
 CONFIG_ARCH_SYNQUACER=y
-CONFIG_TEXT_BASE=0x08200000
+CONFIG_POSITION_INDEPENDENT=y
 CONFIG_SYS_MALLOC_LEN=0x1000000
 CONFIG_SYS_MALLOC_F_LEN=0x400
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0xe0000000
 CONFIG_ENV_SIZE=0x30000
-CONFIG_ENV_OFFSET=0x300000
+CONFIG_ENV_OFFSET=0x580000
 CONFIG_ENV_SECT_SIZE=0x10000
 CONFIG_DM_GPIO=y
 CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox"
-- 
2.34.1

[PATCH v4 6/6] fwu: DeveloperBox: add support for FWU

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Add code to support FWU_MULTI_BANK_UPDATE.
The platform does not have gpt-partition storage for
Banks and MetaData, rather it used SPI-NOR backed
mtd regions for the purpose.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 board/socionext/developerbox/Makefile       |   1 +
 board/socionext/developerbox/developerbox.c |   8 +
 board/socionext/developerbox/fwu_plat.c     |  57 +++++++
 configs/synquacer_developerbox_defconfig    |   8 +
 doc/board/socionext/developerbox.rst        | 155 +++++++++++++++++++-
 include/configs/synquacer.h                 |  10 ++
 6 files changed, 233 insertions(+), 6 deletions(-)
 create mode 100644 board/socionext/developerbox/fwu_plat.c

diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
index 4a46de995a..1acd067a7e 100644
--- a/board/socionext/developerbox/Makefile
+++ b/board/socionext/developerbox/Makefile
@@ -7,3 +7,4 @@
 #
 
 obj-y	:= developerbox.o
+obj-$(CONFIG_FWU_MDATA_MTD) += fwu_plat.o
diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
index 16e14d4f7f..ce2cccf4f0 100644
--- a/board/socionext/developerbox/developerbox.c
+++ b/board/socionext/developerbox/developerbox.c
@@ -20,6 +20,13 @@
 
 #if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
 struct efi_fw_image fw_images[] = {
+#if CONFIG_IS_ENABLED(FWU_MULTI_BANK_UPDATE)
+	{
+		.image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
+		.fw_name = u"DEVELOPERBOX-FIP",
+		.image_index = 1,
+	},
+#else
 	{
 		.image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
 		.fw_name = u"DEVELOPERBOX-UBOOT",
@@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
 		.fw_name = u"DEVELOPERBOX-OPTEE",
 		.image_index = 3,
 	},
+#endif
 };
 
 struct efi_capsule_update_info update_info = {
diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
new file mode 100644
index 0000000000..29b258f86c
--- /dev/null
+++ b/board/socionext/developerbox/fwu_plat.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <efi_loader.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <memalign.h>
+#include <mtd.h>
+
+#define DFU_ALT_BUF_LEN 256
+
+/* Generate dfu_alt_info from partitions */
+void set_dfu_alt_info(char *interface, char *devstr)
+{
+	int ret;
+	struct mtd_info *mtd;
+
+	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
+	memset(buf, 0, sizeof(buf));
+
+	mtd_probe_devices();
+
+	mtd = get_mtd_device_nm("nor1");
+	if (IS_ERR_OR_NULL(mtd))
+		return;
+
+	ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
+	if (ret < 0) {
+		log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
+		return;
+	}
+	log_debug("Make dfu_alt_info: '%s'\n", buf);
+
+	env_set("dfu_alt_info", buf);
+}
+
+int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
+			 efi_guid_t *image_id, u8 *alt_num)
+{
+	return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
+}
+
+void fwu_plat_get_bootidx(uint *boot_idx)
+{
+	int ret;
+	u32 active_idx;
+	u32 *bootidx = boot_idx;
+
+	ret = fwu_get_active_index(&active_idx);
+
+	if (ret < 0)
+		*bootidx = -1;
+
+	*bootidx = active_idx;
+}
diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
index 09e12b739b..d09684153a 100644
--- a/configs/synquacer_developerbox_defconfig
+++ b/configs/synquacer_developerbox_defconfig
@@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
 CONFIG_EFI_CAPSULE_ON_DISK=y
 CONFIG_EFI_IGNORE_OSINDICATIONS=y
 CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
+CONFIG_EFI_SECURE_BOOT=y
+CONFIG_FWU_MULTI_BANK_UPDATE=y
+CONFIG_FWU_MDATA=y
+CONFIG_FWU_MDATA_MTD=y
+CONFIG_FWU_NUM_BANKS=2
+CONFIG_FWU_NUM_IMAGES_PER_BANK=1
+CONFIG_CMD_FWU_METADATA=y
+CONFIG_TOOLS_MKFWUMDATA=y
diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
index 2d943c23be..908d3a7e6f 100644
--- a/doc/board/socionext/developerbox.rst
+++ b/doc/board/socionext/developerbox.rst
@@ -57,14 +57,20 @@ Installation
 
 You can install the SNI_NOR_UBOOT.fd via NOR flash writer.
 
-Flashing the U-Boot image on DeveloperBox requires a 96boards UART mezzanine or other mezzanine which can connect to LS-UART0 port.
-Connect USB cable from host to the LS-UART0 and set DSW2-7 to ON, and turn the board on again. The flash writer program will be started automatically; don’t forget to turn the DSW2-7 off again after flashing.
+Flashing the U-Boot image on DeveloperBox requires a 96boards UART mezzanine
+or other mezzanine which can connect to LS-UART0 port.
+Connect USB cable from host to the LS-UART0 and set DSW2-7 to ON, and turn the
+board on again. The flash writer program will be started automatically;
+don't forget to turn the DSW2-7 off again after flashing.
 
-*!!CAUTION!! If you failed to write the U-Boot image on wrong address, the board can be bricked. See below page if you need to recover the bricked board. See the following page for more detail*
+*!!CAUTION!! If you write the U-Boot image on wrong address, the board can
+be bricked. See below page if you need to recover the bricked board. See
+the following page for more detail*
 
 https://www.96boards.org/documentation/enterprise/developerbox/installation/board-recovery.md.html
 
-When the serial flasher is running correctly is will show the following boot messages shown via LS-UART0::
+When the serial flasher is running correctly is will show the following boot
+messages shown via LS-UART0::
 
 
   /*------------------------------------------*/
@@ -81,7 +87,144 @@ Once the flasher tool is running we are ready flash the UEFI image::
   flash rawwrite 200000 100000
   >> Send SPI_NOR_UBOOT.fd via XMODEM (Control-A S in minicom) <<
 
-*!!NOTE!! The flasher command parameter is different from the command for board recovery. U-Boot uses the offset 200000 (2-five-0, 2M in hex) and the size 100000 (1-five-0, 1M in hex).*
+*!!NOTE!! The flasher command parameter is different from the command for
+board recovery. U-Boot uses the offset 200000 (2-five-0, 2M in hex) and the
+size 100000 (1-five-0, 1M in hex).*
 
-After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
+After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and
+reset the board.
 
+
+Enable FWU Multi Bank Update
+============================
+
+DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both
+*SCP firmware* and *TF-A* for this feature. This will change the layout and
+the boot process but you can switch back to the normal one by changing
+the DSW 1-4 off.
+
+Configure U-Boot
+----------------
+
+To enable the FWU Multi Bank Update on the DeveloperBox, you need to add
+following configurations to configs/synquacer_developerbox_defconfig ::
+
+ CONFIG_FWU_MULTI_BANK_UPDATE=y
+ CONFIG_FWU_MDATA=y
+ CONFIG_FWU_MDATA_MTD=y
+ CONFIG_FWU_NUM_BANKS=2
+ CONFIG_FWU_NUM_IMAGES_PER_BANK=1
+ CONFIG_CMD_FWU_METADATA=y
+
+And build it::
+
+  cd u-boot/
+  export ARCH=arm64
+  export CROSS_COMPILE=aarch64-linux-gnu-
+  make synquacer_developerbox_defconfig
+  make -j `noproc`
+  cd ../
+
+By default, the CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANKS are
+set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image
+which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)
+You can use fiptool to compose the FIP image from those firmware images.
+
+Rebuild SCP firmware
+--------------------
+
+Rebuild SCP firmware which supports FWU Multi Bank Update as below::
+
+  cd SCP-firmware/
+  OUT=./build/product/synquacer
+  ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin
+  RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin
+  ROMRAMFW_FILE=scp_romramfw_release.bin
+
+  make CC=arm-none-eabi-gcc PRODUCT=synquacer MODE=release
+  tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608
+  dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0
+  dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536
+  cd ../
+
+And you can get the `scp_romramfw_release.bin` file
+
+Rebuild OPTEE firmware
+----------------------
+
+Rebuild OPTEE to use in new-layout fip as below::
+
+  cd optee_os/
+  make -j`nproc` PLATFORM=synquacer ARCH=arm \
+    CROSS_COMPILE64=aarch64-linux-gnu- CFG_ARM64_core=y \
+    CFG_CRYPTO_WITH_CE=y CFG_CORE_HEAP_SIZE=524288 CFG_CORE_DYN_SHM=y \
+    CFG_CORE_ARM64_PA_BITS=48 CFG_TEE_CORE_LOG_LEVEL=1 CFG_TEE_TA_LOG_LEVEL=1
+  cp out/arm-plat-synquacer/core/tee-pager_v2.bin ../arm-trusted-firmware/
+
+The produced `tee-pager_v2.bin` is to be used while building TF-A next.
+
+
+Rebuild TF-A and FIP
+--------------------
+
+Rebuild TF-A which supports FWU Multi Bank Update as below::
+
+  cd arm-trusted-firmware/
+  make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \
+     TRUSTED_BOARD_BOOT=1 SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 \
+     MBEDTLS_DIR=../mbedtls BL32=tee-pager_v2.bin \
+     BL33=../u-boot/u-boot.bin all fip fiptool
+
+And make a FIP image.::
+
+  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
+  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
+
+UUIDs for the FWU Multi Bank Update
+-----------------------------------
+
+FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
+following UUIDs.
+
+ - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
+ - Image type UUID for the FIP image: 10c36d7d-ca52-b843-b7b9-f9d6c501d108
+ - Image UUID for Bank0 : 5a66a702-99fd-4fef-a392-c26e261a2828
+ - Image UUID for Bank1 : a8f868a1-6e5c-4757-878d-ce63375ef2c0
+
+These UUIDs are used for making a FWU metadata image.
+
+u-boot$ ./tools/mkfwumdata -i 1 -b 2 \
+	17e86d77-41f9-4fd7-87ec-a55df9842de5,10c36d7d-ca52-b843-b7b9-f9d6c501d108,5a66a702-99fd-4fef-a392-c26e261a2828,a8f868a1-6e5c-4757-878d-ce63375ef2c0 \
+	../devbox-fwu-mdata.img
+
+Create Accept & Revert capsules
+
+u-boot$ ./tools/mkeficapsule -A -g 7d6dc310-52ca-43b8-b7b9-f9d6c501d108 NEWFIP_accept.Cap
+u-boot$ ./tools/mkeficapsule -R NEWFIP_revert.Cap
+
+Install via flash writer
+------------------------
+
+As explained in above section, the new FIP image and the FWU metadata image
+can be installed via NOR flash writer. Note that the installation offsets for
+the FWU multi bank update supported firmware.
+
+Once the flasher tool is running we are ready flash the images.::
+Write the FIP image to the Bank-0 & 1 at 6MB and 10MB offset.::
+
+  flash rawwrite 600000 180000
+  flash rawwrite a00000 180000
+  >> Send SPI_NOR_NEWFIP.fd via XMODEM (Control-A S in minicom) <<
+
+  flash rawwrite 500000 1000
+  flash rawwrite 530000 1000
+  >> Send devbox-fwu-mdata.img via XMODEM (Control-A S in minicom) <<
+
+And write the new SCP firmware.::
+
+  flash write cm3
+  >> Send scp_romramfw_release.bin via XMODEM (Control-A S in minicom) <<
+
+At last, turn on the DSW 3-4 on the board, and reboot.
+Note that if DSW 3-4 is turned off, the DeveloperBox will boot from
+the original EDK2 firmware (or non-FWU U-Boot if you already installed.)
diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h
index 8f44c6f66a..cd7359c2f8 100644
--- a/include/configs/synquacer.h
+++ b/include/configs/synquacer.h
@@ -40,19 +40,29 @@
 
 /* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */
 
+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
+#define DEFAULT_DFU_ALT_INFO
+#else
 #define DEFAULT_DFU_ALT_INFO "dfu_alt_info="				\
 			"mtd nor1=u-boot.bin raw 200000 100000;"	\
 			"fip.bin raw 180000 78000;"			\
 			"optee.bin raw 500000 100000\0"
+#endif
 
 /* GUIDs for capsule updatable firmware images */
 #define DEVELOPERBOX_UBOOT_IMAGE_GUID \
 	EFI_GUID(0x53a92e83, 0x4ef4, 0x473a, 0x8b, 0x0d, \
 		 0xb5, 0xd8, 0xc7, 0xb2, 0xd6, 0x00)
 
+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
+#define DEVELOPERBOX_FIP_IMAGE_GUID \
+	EFI_GUID(0x7d6dc310, 0x52ca, 0x43b8, 0xb7, 0xb9, \
+		 0xf9, 0xd6, 0xc5, 0x01, 0xd1, 0x08)
+#else
 #define DEVELOPERBOX_FIP_IMAGE_GUID \
 	EFI_GUID(0x880866e9, 0x84ba, 0x4793, 0xa9, 0x08, \
 		 0x33, 0xe0, 0xb9, 0x16, 0xf3, 0x98)
+#endif
 
 #define DEVELOPERBOX_OPTEE_IMAGE_GUID \
 	EFI_GUID(0xc1b629f1, 0xce0e, 0x4894, 0x82, 0xbf, \
-- 
2.34.1

Re: [PATCH v4 4/6] dt: fwu: developerbox: enable fwu banks and mdata regions

[Michal Simek]

On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Specify Bank-0/1 and fwu metadata mtd regions.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   .../synquacer-sc2a11-developerbox-u-boot.dtsi | 49 +++++++++++++++++--
>   1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> index 9f9837b33b..9957646a46 100644
> --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> @@ -21,7 +21,7 @@
>   		#size-cells = <0>;
>   		status = "okay";
>   
> -		flash@0 {
> +		flash0: flash@0 {
>   			#address-cells = <1>;
>   			#size-cells = <1>;
>   			compatible = "jedec,spi-nor";
> @@ -74,8 +74,24 @@
>   				};
>   
>   				partition@500000 {
> -					label = "Ex-OPTEE";
> -					reg = <0x500000 0x200000>;
> +					label = "MDATA-Pri";
> +					reg = <0x500000 0x1000>;
> +				};
> +
> +				partition@530000 {
> +					label = "MDATA-Sec";
> +					reg = <0x530000 0x1000>;
> +				};
> +
> +				/* FWU Multi bank update partitions */
> +				partition@600000 {
> +					label = "FIP-Bank0";
> +					reg = <0x600000 0x400000>;
> +				};
> +
> +				partition@a00000 {
> +					label = "FIP-Bank1";
> +					reg = <0xa00000 0x400000>;
>   				};
>   			};
>   		};
> @@ -102,6 +118,33 @@
>   		optee {
>   			status = "okay";
>   		};
> +
> +		fwu-mdata {
> +			compatible = "u-boot,fwu-mdata-mtd";
> +			fwu-mdata-store = <&flash0>;
> +			mdata-parts = "MDATA-Pri", "MDATA-Sec";

As I discussed this with Ilias today. This should be approved or this DT won't 
pass yaml checking for SR certification. That's why this should get to schema to 
be able to widely use.

Thanks,
Michal

Re: [PATCH v4 2/6] FWU: mtd: Add helper functions for accessing FWU metadata

[Michal Simek]

On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> 
> Add helper functions needed for accessing the FWU metadata which
> contains information on the updatable images.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   include/fwu.h             |  34 ++++++++
>   lib/fwu_updates/Makefile  |   1 +
>   lib/fwu_updates/fwu_mtd.c | 164 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 199 insertions(+)
>   create mode 100644 lib/fwu_updates/fwu_mtd.c
> 
> diff --git a/include/fwu.h b/include/fwu.h
> index 33b4d0b3db..117f51c4f5 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -8,6 +8,8 @@
>   
>   #include <blk.h>
>   #include <efi.h>
> +#include <mtd.h>
> +#include <uuid.h>
>   
>   #include <linux/types.h>
>   
> @@ -18,6 +20,12 @@ struct fwu_mdata_gpt_blk_priv {
>   	struct udevice *blk_dev;
>   };
>   
> +struct fwu_mtd_image_info {
> +	u32 start, size;
> +	int bank_num, image_num;
> +	char uuidbuf[UUID_STR_LEN + 1];
> +};
> +
>   struct fwu_mdata_ops {
>   	/**
>   	 * read_mdata() - Populate the asked FWU metadata copy
> @@ -251,4 +259,30 @@ u8 fwu_empty_capsule_checks_pass(void);
>    */
>   int fwu_trial_state_ctr_start(void);
>   
> +/**
> + * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
> + * @buf: Buffer into which the dfu_alt_info is filled
> + * @len: Maximum characters that can be written in buf
> + * @mtd: Pointer to underlying MTD device
> + *
> + * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
> + *
> + * Return: 0 if OK, -ve on error
> + *

nit: remove this line above.

> + */
> +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
> +
> +/**
> + * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
> + * @image_guid: Image GUID for which DFU alt number needs to be retrieved

it doesn't match with parameter below.

> + * @alt_num: Pointer to the alt_num
> + * @mtd_dev: Name of mtd device instance
> + *
> + * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
> + *
> + * Return: 0 if OK, -ve on error
> + *

nit: remove this line above

> + */
> +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char *mtd_dev);


./scripts/kernel-doc -v -man include/fwu.h 1>/dev/null

include/fwu.h:276: info: Scanning doc for fwu_mtd_get_alt_num
include/fwu.h:286: warning: Function parameter or member 'image_id' not 
described in 'fwu_mtd_get_alt_num'
include/fwu.h:286: warning: Excess function parameter 'image_guid' description 
in 'fwu_mtd_get_alt_num'

(In general this file has other kernel-doc issues which should be fixed too)


> +
>   #endif /* _FWU_H_ */
> diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
> index 1993088e5b..c9e3c06b48 100644
> --- a/lib/fwu_updates/Makefile
> +++ b/lib/fwu_updates/Makefile
> @@ -5,3 +5,4 @@
>   
>   obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
>   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mtd.o
> diff --git a/lib/fwu_updates/fwu_mtd.c b/lib/fwu_updates/fwu_mtd.c
> new file mode 100644
> index 0000000000..c1d04e574b
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_mtd.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <dm.h>
> +#include <dfu.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <mtd.h>
> +#include <uuid.h>
> +#include <vsprintf.h>
> +
> +#include <dm/ofnode.h>
> +
> +struct fwu_mtd_image_info
> +fwu_mtd_images[CONFIG_FWU_NUM_BANKS * CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +
> +static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
> +{
> +	int num_images = sizeof(fwu_mtd_images) / sizeof(struct fwu_mtd_image_info);

include/linux/kernel.h:55:#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))


> +
> +	for (int i = 0; i < num_images; i++)
> +		if (!strcmp(uuidbuf, fwu_mtd_images[i].uuidbuf))
> +			return &fwu_mtd_images[i];
> +
> +	return NULL;
> +}
> +
> +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
> +			const char *mtd_dev)
> +{
> +	int i, nalt;
> +	int ret = -1;

nit: can be on the same line.

> +	struct mtd_info *mtd;
> +	struct dfu_entity *dfu;
> +	fdt_addr_t offset, size = 0;
> +	char uuidbuf[UUID_STR_LEN + 1];
> +	struct fwu_mtd_image_info *mtd_img_info;

nit: reverse christmas tree order

> +
> +	mtd_probe_devices();
> +	mtd = get_mtd_device_nm(mtd_dev);

you are getting link but you are not using it anywhere.
You should check return value or remove this call completely.


> +
> +	uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
> +
> +	mtd_img_info = mtd_img_by_uuid(uuidbuf);
> +	if (!mtd_img_info) {
> +		log_err("%s: Not found partition for image %s\n", __func__, uuidbuf);
> +		return -ENOENT;
> +	}

nit: newline

> +	offset = mtd_img_info->start;
> +	size = mtd_img_info->size;
> +
> +	dfu_init_env_entities(NULL, NULL);

worth to check return value here.

M

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Michal Simek]

On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> 
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, on a separate
> region. Add a driver for reading from and writing to the metadata
> when the updatable images and the metadata are stored on a raw
> MTD region.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   drivers/fwu-mdata/Kconfig   |  15 ++
>   drivers/fwu-mdata/Makefile  |   1 +
>   drivers/fwu-mdata/raw_mtd.c | 272 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 288 insertions(+)
>   create mode 100644 drivers/fwu-mdata/raw_mtd.c
> 
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> index 36c4479a59..42736a5e43 100644
> --- a/drivers/fwu-mdata/Kconfig
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -6,6 +6,11 @@ config FWU_MDATA
>   	  FWU Metadata partitions reside on the same storage device
>   	  which contains the other FWU updatable firmware images.
>   
> +choice
> +	prompt "Storage Layout Scheme"
> +	depends on FWU_MDATA
> +	default FWU_MDATA_GPT_BLK
> +
>   config FWU_MDATA_GPT_BLK
>   	bool "FWU Metadata access for GPT partitioned Block devices"
>   	select PARTITION_TYPE_GUID
> @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
>   	help
>   	  Enable support for accessing FWU Metadata on GPT partitioned
>   	  block devices.
> +
> +config FWU_MDATA_MTD
> +	bool "Raw MTD devices"
> +	depends on MTD
> +	help
> +	  Enable support for accessing FWU Metadata on non-partitioned
> +	  (or non-GPT partitioned, e.g. partition nodes in devicetree)
> +	  MTD devices.
> +
> +endchoice
> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> index 3fee64c10c..06c49747ba 100644
> --- a/drivers/fwu-mdata/Makefile
> +++ b/drivers/fwu-mdata/Makefile
> @@ -6,3 +6,4 @@
>   
>   obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
>   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> new file mode 100644
> index 0000000000..4b1a10073a
> --- /dev/null
> +++ b/drivers/fwu-mdata/raw_mtd.c
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0+

Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?

> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#define LOG_CATEGORY UCLASS_FWU_MDATA
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
> +/* Internal helper structure to move data around */
> +struct fwu_mdata_mtd_priv {
> +	struct mtd_info *mtd;
> +	char pri_label[50];
> +	char sec_label[50];
> +	u32 pri_offset;
> +	u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> +	FWU_MTD_READ,
> +	FWU_MTD_WRITE,
> +};
> +
> +extern struct fwu_mtd_image_info fwu_mtd_images[];

if there is a need to share it. It should go to header.

And fwu_mtd_images[] is not defined when only this patch is applied.
It means order of patches is not right. 1/6 and 2/6 should be swapped


> +
> +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> +{
> +	return !do_div(size, mtd->erasesize);
> +}
> +
> +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> +		       enum fwu_mtd_op op)
> +{
> +	struct mtd_oob_ops io_op = {};
> +	u64 lock_offs, lock_len;
> +	size_t len;
> +	void *buf;
> +	int ret;
> +
> +	if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> +		log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
> +		return -EINVAL;
> +	}
> +
> +	lock_offs = offs;
> +	/* This will expand erase size to align with the block size */
> +	lock_len = round_up(size, mtd->erasesize);
> +
> +	ret = mtd_unlock(mtd, lock_offs, lock_len);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		struct erase_info erase_op = {};
> +
> +		erase_op.mtd = mtd;
> +		erase_op.addr = lock_offs;
> +		erase_op.len = lock_len;
> +		erase_op.scrub = 0;
> +
> +		ret = mtd_erase(mtd, &erase_op);
> +		if (ret)
> +			goto lock;
> +	}
> +
> +	/* Also, expand the write size to align with the write size */
> +	len = round_up(size, mtd->writesize);
> +
> +	buf = memalign(ARCH_DMA_MINALIGN, len);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto lock;
> +	}
> +	memset(buf, 0xff, len);
> +
> +	io_op.mode = MTD_OPS_AUTO_OOB;
> +	io_op.len = len;
> +	io_op.ooblen = 0;
> +	io_op.datbuf = buf;
> +	io_op.oobbuf = NULL;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		memcpy(buf, data, size);
> +		ret = mtd_write_oob(mtd, offs, &io_op);
> +	} else {
> +		ret = mtd_read_oob(mtd, offs, &io_op);
> +		if (!ret)
> +			memcpy(data, buf, size);
> +	}
> +	free(buf);
> +
> +lock:
> +	mtd_lock(mtd, lock_offs, lock_len);
> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	struct mtd_info *mtd = mtd_priv->mtd;
> +	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> +	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
> +}
> +
> +static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	struct mtd_info *mtd = mtd_priv->mtd;
> +	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> +	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +static int flash_partition_offset(struct udevice *dev, const char *part_name, fdt_addr_t *offset)
> +{
> +	ofnode node, parts_node;
> +	fdt_addr_t size = 0;
> +
> +	parts_node = ofnode_by_compatible(dev_ofnode(dev), "fixed-partitions");
> +	node = ofnode_by_prop_value(parts_node, "label", part_name, strlen(part_name) + 1);
> +	if (!ofnode_valid(node)) {
> +		log_err("Warning: Failed to find partition by label <%s>\n", part_name);
> +		return -ENOENT;
> +	}
> +
> +	*offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
> +
> +	return (int)size;
> +}
> +
> +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	const fdt32_t *phandle_p = NULL;
> +	struct udevice *mtd_dev;
> +	struct mtd_info *mtd;
> +	const char *label;
> +	fdt_addr_t offset;
> +	int ret, size;
> +	u32 phandle;
> +	ofnode bank;
> +	int off_img;
> +
> +	/* Find the FWU mdata storage device */
> +	phandle_p = ofnode_get_property(dev_ofnode(dev),
> +					"fwu-mdata-store", &size);
> +	if (!phandle_p) {
> +		log_err("FWU meta data store not defined in device-tree\n");
> +		return -ENOENT;
> +	}
> +
> +	phandle = fdt32_to_cpu(*phandle_p);
> +
> +	ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> +					  &mtd_dev);
> +	if (ret) {
> +		log_err("FWU: failed to get mtd device\n");
> +		return ret;
> +	}
> +
> +	mtd_probe_devices();
> +
> +	mtd_for_each_device(mtd) {
> +		if (mtd->dev == mtd_dev) {
> +			mtd_priv->mtd = mtd;
> +			log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
> +			break;
> +		}
> +	}
> +	if (!mtd_priv->mtd) {
> +		log_err("Failed to find mtd device by fwu-mdata-store\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Get the offset of primary and seconday mdata */


typo


> +	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0, &label);
> +	if (ret)
> +		return ret;
> +	strcpy(mtd_priv->pri_label, label);
> +
> +	ret = flash_partition_offset(mtd_dev, mtd_priv->pri_label, &offset);
> +	if (ret <= 0)
> +		return ret;
> +	mtd_priv->pri_offset = offset;
> +
> +	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 1, &label);
> +	if (ret)
> +		return ret;
> +	strcpy(mtd_priv->sec_label, label);
> +
> +	ret = flash_partition_offset(mtd_dev, mtd_priv->sec_label, &offset);
> +	if (ret <= 0)
> +		return ret;
> +	mtd_priv->sec_offset = offset;
> +
> +	off_img = 0;
> +
> +	ofnode_for_each_subnode(bank, dev_ofnode(dev)) {
> +		int bank_num, bank_offset, bank_size;
> +		const char *bank_name;
> +		ofnode image;
> +
> +		ofnode_read_u32(bank, "id", &bank_num);
> +		bank_name = ofnode_read_string(bank, "label");
> +		bank_size = flash_partition_offset(mtd_dev, bank_name, &offset);
> +		if (bank_size <= 0)
> +			return bank_size;
> +		bank_offset = offset;
> +		log_debug("Bank%d: %s [0x%x - 0x%x]\n",
> +			  bank_num, bank_name, bank_offset, bank_offset + bank_size);
> +
> +		ofnode_for_each_subnode(image, bank) {
> +			int image_num, image_offset, image_size;
> +			const char *uuid;
> +
> +			if (off_img == CONFIG_FWU_NUM_BANKS *
> +						CONFIG_FWU_NUM_IMAGES_PER_BANK) {
> +				log_err("DT provides images more than configured!\n");
> +				break;
> +			}
> +
> +			uuid = ofnode_read_string(image, "uuid");
> +			ofnode_read_u32(image, "id", &image_num);
> +			ofnode_read_u32(image, "offset", &image_offset);
> +			ofnode_read_u32(image, "size", &image_size);
> +
> +			fwu_mtd_images[off_img].start = bank_offset + image_offset;
> +			fwu_mtd_images[off_img].size = image_size;
> +			fwu_mtd_images[off_img].bank_num = bank_num;
> +			fwu_mtd_images[off_img].image_num = image_num;
> +			strcpy(fwu_mtd_images[off_img].uuidbuf, uuid);
> +			log_debug("\tImage%d: %s @0x%x\n\n",
> +				  image_num, uuid, bank_offset + image_offset);
> +			off_img++;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int fwu_mdata_mtd_probe(struct udevice *dev)
> +{
> +	/* Ensure the metadata can be read. */
> +	return fwu_get_mdata(NULL);
> +}
> +
> +static struct fwu_mdata_ops fwu_mtd_ops = {
> +	.read_mdata = fwu_mtd_read_mdata,
> +	.write_mdata = fwu_mtd_write_mdata,
> +};
> +
> +static const struct udevice_id fwu_mdata_ids[] = {
> +	{ .compatible = "u-boot,fwu-mdata-mtd" },
> +	{ }
> +};

As I said this DT binding should be approved first to make sure that we don't 
need to fix DT binding in future. Just simply do it right from the begining.

Thanks,
Michal

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> 
> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> partition to be used in A/B Update imeplementation.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   tools/Kconfig      |   9 ++
>   tools/Makefile     |   4 +
>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 347 insertions(+)
>   create mode 100644 tools/mkfwumdata.c
> 
> diff --git a/tools/Kconfig b/tools/Kconfig
> index 539708f277..6e23f44d55 100644
> --- a/tools/Kconfig
> +++ b/tools/Kconfig
> @@ -157,4 +157,13 @@ config LUT_SEQUENCE
>   	help
>   	  Look Up Table Sequence
>   
> +config TOOLS_MKFWUMDATA
> +	bool "Build mkfwumdata command"
> +	default y if FWU_MULTI_BANK_UPDATE
> +	help
> +	  This command allows users to create a raw image of the FWU
> +	  metadata for initial installation of the FWU multi bank
> +	  update on the board. The installation method depends on
> +	  the platform.
> +
>   endmenu
> diff --git a/tools/Makefile b/tools/Makefile
> index e13effbb66..80eee71505 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -247,6 +247,10 @@ HOSTLDLIBS_mkeficapsule += \
>   	$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
>   hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
>   
> +mkfwumdata-objs := mkfwumdata.o lib/crc32.o
> +HOSTLDLIBS_mkfwumdata += -luuid
> +hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
> +
>   # We build some files with extra pedantic flags to try to minimize things
>   # that won't build on some weird host compiler -- though there are lots of
>   # exceptions for files that aren't complaint.
> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> new file mode 100644
> index 0000000000..43dabf3b72
> --- /dev/null
> +++ b/tools/mkfwumdata.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <limits.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <u-boot/crc.h>
> +#include <unistd.h>
> +#include <uuid/uuid.h>
> +
> +/* This will dynamically allocate the fwu_mdata */
> +#define CONFIG_FWU_NUM_BANKS		0
> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0

These two are completely unused.

Thanks,
Michal

Re: [PATCH v4 6/6] fwu: DeveloperBox: add support for FWU

[Michal Simek]

On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Add code to support FWU_MULTI_BANK_UPDATE.
> The platform does not have gpt-partition storage for
> Banks and MetaData, rather it used SPI-NOR backed
> mtd regions for the purpose.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   board/socionext/developerbox/Makefile       |   1 +
>   board/socionext/developerbox/developerbox.c |   8 +
>   board/socionext/developerbox/fwu_plat.c     |  57 +++++++
>   configs/synquacer_developerbox_defconfig    |   8 +
>   doc/board/socionext/developerbox.rst        | 155 +++++++++++++++++++-
>   include/configs/synquacer.h                 |  10 ++
>   6 files changed, 233 insertions(+), 6 deletions(-)
>   create mode 100644 board/socionext/developerbox/fwu_plat.c
> 
> diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
> index 4a46de995a..1acd067a7e 100644
> --- a/board/socionext/developerbox/Makefile
> +++ b/board/socionext/developerbox/Makefile
> @@ -7,3 +7,4 @@
>   #
>   
>   obj-y	:= developerbox.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += fwu_plat.o
> diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
> index 16e14d4f7f..ce2cccf4f0 100644
> --- a/board/socionext/developerbox/developerbox.c
> +++ b/board/socionext/developerbox/developerbox.c
> @@ -20,6 +20,13 @@
>   
>   #if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
>   struct efi_fw_image fw_images[] = {
> +#if CONFIG_IS_ENABLED(FWU_MULTI_BANK_UPDATE)
> +	{
> +		.image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> +		.fw_name = u"DEVELOPERBOX-FIP",
> +		.image_index = 1,
> +	},
> +#else
>   	{
>   		.image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
>   		.fw_name = u"DEVELOPERBOX-UBOOT",
> @@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
>   		.fw_name = u"DEVELOPERBOX-OPTEE",
>   		.image_index = 3,
>   	},
> +#endif
>   };
>   
>   struct efi_capsule_update_info update_info = {
> diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
> new file mode 100644
> index 0000000000..29b258f86c
> --- /dev/null
> +++ b/board/socionext/developerbox/fwu_plat.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <efi_loader.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.h>
> +#include <mtd.h>
> +
> +#define DFU_ALT_BUF_LEN 256
> +
> +/* Generate dfu_alt_info from partitions */
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	int ret;
> +	struct mtd_info *mtd;
> +
> +	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> +	memset(buf, 0, sizeof(buf));
> +
> +	mtd_probe_devices();
> +
> +	mtd = get_mtd_device_nm("nor1");
> +	if (IS_ERR_OR_NULL(mtd))
> +		return;
> +
> +	ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
> +	if (ret < 0) {
> +		log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
> +		return;
> +	}
> +	log_debug("Make dfu_alt_info: '%s'\n", buf);
> +
> +	env_set("dfu_alt_info", buf);
> +}
> +
> +int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
> +			 efi_guid_t *image_id, u8 *alt_num)
> +{
> +	return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> +}
> +
> +void fwu_plat_get_bootidx(uint *boot_idx)
> +{
> +	int ret;
> +	u32 active_idx;
> +	u32 *bootidx = boot_idx;
> +
> +	ret = fwu_get_active_index(&active_idx);
> +

nit: remove this newline

> +	if (ret < 0)
> +		*bootidx = -1;
> +
> +	*bootidx = active_idx;

Is this logic here right?
If fwu_get_active_index fails you setup bootidx to -1
and right after it you rewrite it to active_idx initialized in 
fwu_get_active_index() to mdata->active_index.

It means why to do *bootidx = -1; at all?

> +}
> diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
> index 09e12b739b..d09684153a 100644
> --- a/configs/synquacer_developerbox_defconfig
> +++ b/configs/synquacer_developerbox_defconfig
> @@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>   CONFIG_EFI_CAPSULE_ON_DISK=y
>   CONFIG_EFI_IGNORE_OSINDICATIONS=y
>   CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> +CONFIG_EFI_SECURE_BOOT=y
> +CONFIG_FWU_MULTI_BANK_UPDATE=y
> +CONFIG_FWU_MDATA=y
> +CONFIG_FWU_MDATA_MTD=y
> +CONFIG_FWU_NUM_BANKS=2
> +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> +CONFIG_CMD_FWU_METADATA=y
> +CONFIG_TOOLS_MKFWUMDATA=y

doesn't look like that it was created via savedefconfig.

> diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
> index 2d943c23be..908d3a7e6f 100644
> --- a/doc/board/socionext/developerbox.rst
> +++ b/doc/board/socionext/developerbox.rst
> @@ -57,14 +57,20 @@ Installation
>   
>   You can install the SNI_NOR_UBOOT.fd via NOR flash writer.
>   
> -Flashing the U-Boot image on DeveloperBox requires a 96boards UART mezzanine or other mezzanine which can connect to LS-UART0 port.
> -Connect USB cable from host to the LS-UART0 and set DSW2-7 to ON, and turn the board on again. The flash writer program will be started automatically; don’t forget to turn the DSW2-7 off again after flashing.
> +Flashing the U-Boot image on DeveloperBox requires a 96boards UART mezzanine
> +or other mezzanine which can connect to LS-UART0 port.
> +Connect USB cable from host to the LS-UART0 and set DSW2-7 to ON, and turn the
> +board on again. The flash writer program will be started automatically;
> +don't forget to turn the DSW2-7 off again after flashing.
>   
> -*!!CAUTION!! If you failed to write the U-Boot image on wrong address, the board can be bricked. See below page if you need to recover the bricked board. See the following page for more detail*
> +*!!CAUTION!! If you write the U-Boot image on wrong address, the board can
> +be bricked. See below page if you need to recover the bricked board. See
> +the following page for more detail*
>   
>   https://www.96boards.org/documentation/enterprise/developerbox/installation/board-recovery.md.html
>   
> -When the serial flasher is running correctly is will show the following boot messages shown via LS-UART0::
> +When the serial flasher is running correctly is will show the following boot
> +messages shown via LS-UART0::
>   
>   
>     /*------------------------------------------*/
> @@ -81,7 +87,144 @@ Once the flasher tool is running we are ready flash the UEFI image::
>     flash rawwrite 200000 100000
>     >> Send SPI_NOR_UBOOT.fd via XMODEM (Control-A S in minicom) <<
>   
> -*!!NOTE!! The flasher command parameter is different from the command for board recovery. U-Boot uses the offset 200000 (2-five-0, 2M in hex) and the size 100000 (1-five-0, 1M in hex).*
> +*!!NOTE!! The flasher command parameter is different from the command for
> +board recovery. U-Boot uses the offset 200000 (2-five-0, 2M in hex) and the
> +size 100000 (1-five-0, 1M in hex).*
>   
> -After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
> +After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and
> +reset the board.
>   
> +
> +Enable FWU Multi Bank Update
> +============================
> +
> +DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both
> +*SCP firmware* and *TF-A* for this feature. This will change the layout and
> +the boot process but you can switch back to the normal one by changing
> +the DSW 1-4 off.
> +
> +Configure U-Boot
> +----------------
> +
> +To enable the FWU Multi Bank Update on the DeveloperBox, you need to add
> +following configurations to configs/synquacer_developerbox_defconfig ::
> +
> + CONFIG_FWU_MULTI_BANK_UPDATE=y
> + CONFIG_FWU_MDATA=y
> + CONFIG_FWU_MDATA_MTD=y
> + CONFIG_FWU_NUM_BANKS=2
> + CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> + CONFIG_CMD_FWU_METADATA=y
> +
> +And build it::
> +
> +  cd u-boot/
> +  export ARCH=arm64
> +  export CROSS_COMPILE=aarch64-linux-gnu-
> +  make synquacer_developerbox_defconfig
> +  make -j `noproc`
> +  cd ../
> +
> +By default, the CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANKS are
> +set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image
> +which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)
> +You can use fiptool to compose the FIP image from those firmware images.
> +
> +Rebuild SCP firmware
> +--------------------
> +
> +Rebuild SCP firmware which supports FWU Multi Bank Update as below::
> +
> +  cd SCP-firmware/
> +  OUT=./build/product/synquacer
> +  ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin
> +  RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin
> +  ROMRAMFW_FILE=scp_romramfw_release.bin
> +
> +  make CC=arm-none-eabi-gcc PRODUCT=synquacer MODE=release
> +  tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608
> +  dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0
> +  dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536
> +  cd ../
> +
> +And you can get the `scp_romramfw_release.bin` file
> +
> +Rebuild OPTEE firmware
> +----------------------
> +
> +Rebuild OPTEE to use in new-layout fip as below::
> +
> +  cd optee_os/
> +  make -j`nproc` PLATFORM=synquacer ARCH=arm \
> +    CROSS_COMPILE64=aarch64-linux-gnu- CFG_ARM64_core=y \
> +    CFG_CRYPTO_WITH_CE=y CFG_CORE_HEAP_SIZE=524288 CFG_CORE_DYN_SHM=y \
> +    CFG_CORE_ARM64_PA_BITS=48 CFG_TEE_CORE_LOG_LEVEL=1 CFG_TEE_TA_LOG_LEVEL=1
> +  cp out/arm-plat-synquacer/core/tee-pager_v2.bin ../arm-trusted-firmware/
> +
> +The produced `tee-pager_v2.bin` is to be used while building TF-A next.
> +
> +
> +Rebuild TF-A and FIP
> +--------------------
> +
> +Rebuild TF-A which supports FWU Multi Bank Update as below::
> +
> +  cd arm-trusted-firmware/
> +  make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \
> +     TRUSTED_BOARD_BOOT=1 SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 \
> +     MBEDTLS_DIR=../mbedtls BL32=tee-pager_v2.bin \
> +     BL33=../u-boot/u-boot.bin all fip fiptool
> +
> +And make a FIP image.::
> +
> +  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
> +  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
> +
> +UUIDs for the FWU Multi Bank Update
> +-----------------------------------
> +
> +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
> +following UUIDs.
> +
> + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5


In past you have it listed at flash node in DT. I see you have removed it 
between v3 and v4 without any note about it.
Is it still needed? And should it be listed in DT spec again?

Thanks,
Michal

Re: [PATCH v4 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[Michal Simek]

On 3/27/23 23:14, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Introduce support for mtd backed storage for FWU feature and enable it on
> Synquacer platform based DeveloperBox.
> 
> This revision is rebased onto patchset that trims the FWU api
>   https://lore.kernel.org/u-boot/20230306231747.1888513-1-jassisinghbrar@gmail.com/

When I build it on the top of this series for our SOC I am getting

    aarch64:  +   xilinx_zynqmp_virt
+lib/fwu_updates/fwu.o: In function `fwu_boottime_checks':
+build/../lib/fwu_updates/fwu.c:633: undefined reference to `fwu_plat_get_bootidx'
+lib/fwu_updates/fwu.o: In function `fwu_get_image_index':
+build/../lib/fwu_updates/fwu.c:381: undefined reference to `fwu_plat_get_alt_num'
+make[1]: *** [Makefile:1754: u-boot] Error 139
+make[1]: *** Deleting file 'u-boot'
+make: *** [Makefile:177: sub-make] Error 2

That means that I can select these options and I will get compilation error.
I understand the error but if anybody can select it you should use weak 
functions or so to be able to get it at least build.

The series doesn't look bad but still the biggest issue remains which is that DT 
binding is not approved by DT guys which pretty much means that any platform 
which starts to use this code can't pass DT yaml checking enforced by SR 2.0.

Thanks,
Michal

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Simon Glass]

Hi,

On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
>
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>
> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> partition to be used in A/B Update imeplementation.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  tools/Kconfig      |   9 ++
>  tools/Makefile     |   4 +
>  tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 347 insertions(+)
>  create mode 100644 tools/mkfwumdata.c

Can you please look at putting this in binman instead, since we would
rather not have another tool with no tests.

Regards,
Simon

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Heinrich Schuchardt]

Am 27. März 2023 23:16:10 MESZ schrieb jassisinghbrar@gmail.com:
>From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>
>Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
>partition to be used in A/B Update imeplementation.
>
>Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>---
> tools/Kconfig      |   9 ++
> tools/Makefile     |   4 +
> tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 347 insertions(+)
> create mode 100644 tools/mkfwumdata.c

For each command line tool there should be a man-page.


>
>diff --git a/tools/Kconfig b/tools/Kconfig
>index 539708f277..6e23f44d55 100644
>--- a/tools/Kconfig
>+++ b/tools/Kconfig
>@@ -157,4 +157,13 @@ config LUT_SEQUENCE
> 	help
> 	  Look Up Table Sequence
> 
>+config TOOLS_MKFWUMDATA
>+	bool "Build mkfwumdata command"
>+	default y if FWU_MULTI_BANK_UPDATE
>+	help
>+	  This command allows users to create a raw image of the FWU
>+	  metadata for initial installation of the FWU multi bank
>+	  update on the board. The installation method depends on
>+	  the platform.
>+
> endmenu
>diff --git a/tools/Makefile b/tools/Makefile
>index e13effbb66..80eee71505 100644
>--- a/tools/Makefile
>+++ b/tools/Makefile
>@@ -247,6 +247,10 @@ HOSTLDLIBS_mkeficapsule += \
> 	$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
> hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> 
>+mkfwumdata-objs := mkfwumdata.o lib/crc32.o
>+HOSTLDLIBS_mkfwumdata += -luuid
>+hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
>+
> # We build some files with extra pedantic flags to try to minimize things
> # that won't build on some weird host compiler -- though there are lots of
> # exceptions for files that aren't complaint.
>diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>new file mode 100644
>index 0000000000..43dabf3b72
>--- /dev/null
>+++ b/tools/mkfwumdata.c
>@@ -0,0 +1,334 @@
>+// SPDX-License-Identifier: GPL-2.0+
>+/*
>+ * Copyright (c) 2023, Linaro Limited
>+ */
>+
>+#include <errno.h>
>+#include <getopt.h>
>+#include <limits.h>
>+#include <stdio.h>
>+#include <stdint.h>
>+#include <stdlib.h>
>+#include <string.h>
>+#include <u-boot/crc.h>
>+#include <unistd.h>
>+#include <uuid/uuid.h>
>+
>+/* This will dynamically allocate the fwu_mdata */
>+#define CONFIG_FWU_NUM_BANKS		0
>+#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0
>+
>+/* Since we can not include fwu.h, redefine version here. */
>+#define FWU_MDATA_VERSION		1
>+
>+typedef uint8_t u8;
>+typedef int16_t s16;
>+typedef uint16_t u16;
>+typedef uint32_t u32;
>+typedef uint64_t u64;
>+
>+#include <fwu_mdata.h>
>+
>+/* TODO: Endianness conversion may be required for some arch. */

Does the update work without UEFI? UEFI requires low endianness.

>+
>+static const char *opts_short = "b:i:a:p:gh";
>+
>+static struct option options[] = {
>+	{"banks", required_argument, NULL, 'b'},
>+	{"images", required_argument, NULL, 'i'},
>+	{"guid", required_argument, NULL, 'g'},
>+	{"active-bank", required_argument, NULL, 'a'},
>+	{"previous-bank", required_argument, NULL, 'p'},
>+	{"help", no_argument, NULL, 'h'},
>+	{NULL, 0, NULL, 0},
>+};
>+
>+static void print_usage(void)
>+{
>+	fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\n");
>+	fprintf(stderr, "Options:\n"
>+		"\t-i, --images <num>          Number of images\n"
>+		"\t-b, --banks  <num>          Number of banks\n"
>+		"\t-a, --active-bank  <num>    Active bank\n"
>+		"\t-p, --previous-bank  <num>  Previous active bank\n"
>+		"\t-g, --guid                  Use GUID instead of UUID\n"

I would not know what this means.

Why can't you supply that single GUID in the position of the UUIDs parameter?

Best regards

Heinrich


>+		"\t-h, --help                  print a help message\n"
>+		);
>+	fprintf(stderr, "  UUIDs list syntax:\n"
>+		"\t  <location uuid>,<image type uuid>,<images uuid list>\n"
>+		"\t     images uuid list syntax:\n"
>+		"\t        img_uuid_00,img_uuid_01...img_uuid_0b,\n"
>+		"\t        img_uuid_10,img_uuid_11...img_uuid_1b,\n"
>+		"\t        ...,\n"
>+		"\t        img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
>+		"\t          where 'b' and 'i' are number of banks and number\n"
>+		"\t          of images in a bank respectively.\n"
>+	       );
>+}
>+
>+struct fwu_mdata_object {
>+	size_t images;
>+	size_t banks;
>+	size_t size;
>+	struct fwu_mdata *mdata;
>+};
>+
>+static int previous_bank, active_bank;
>+static bool __use_guid;
>+
>+static struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
>+{
>+	struct fwu_mdata_object *mobj;
>+
>+	mobj = calloc(1, sizeof(*mobj));
>+	if (!mobj)
>+		return NULL;
>+
>+	mobj->size = sizeof(struct fwu_mdata) +
>+		(sizeof(struct fwu_image_entry) +
>+		 sizeof(struct fwu_image_bank_info) * banks) * images;
>+	mobj->images = images;
>+	mobj->banks = banks;
>+
>+	mobj->mdata = calloc(1, mobj->size);
>+	if (!mobj->mdata) {
>+		free(mobj);
>+		return NULL;
>+	}
>+
>+	return mobj;
>+}
>+
>+static struct fwu_image_entry *
>+fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)
>+{
>+	size_t offset;
>+
>+	offset = sizeof(struct fwu_mdata) +
>+		(sizeof(struct fwu_image_entry) +
>+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
>+
>+	return (struct fwu_image_entry *)((char *)mobj->mdata + offset);
>+}
>+
>+static struct fwu_image_bank_info *
>+fwu_get_bank(struct fwu_mdata_object *mobj, size_t img_idx, size_t bnk_idx)
>+{
>+	size_t offset;
>+
>+	offset = sizeof(struct fwu_mdata) +
>+		(sizeof(struct fwu_image_entry) +
>+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
>+		sizeof(struct fwu_image_entry) +
>+		sizeof(struct fwu_image_bank_info) * bnk_idx;
>+
>+	return (struct fwu_image_bank_info *)((char *)mobj->mdata + offset);
>+}
>+
>+/**
>+ * convert_uuid_to_guid() - convert UUID to GUID
>+ * @buf:	UUID binary
>+ *
>+ * UUID and GUID have the same data structure, but their binary
>+ * formats are different due to the endianness. See lib/uuid.c.
>+ * Since uuid_parse() can handle only UUID, this function must
>+ * be called to get correct data for GUID when parsing a string.
>+ *
>+ * The correct data will be returned in @buf.
>+ */
>+static void convert_uuid_to_guid(unsigned char *buf)
>+{
>+	unsigned char c;
>+
>+	c = buf[0];
>+	buf[0] = buf[3];
>+	buf[3] = c;
>+	c = buf[1];
>+	buf[1] = buf[2];
>+	buf[2] = c;
>+
>+	c = buf[4];
>+	buf[4] = buf[5];
>+	buf[5] = c;
>+
>+	c = buf[6];
>+	buf[6] = buf[7];
>+	buf[7] = c;
>+}
>+
>+static int uuid_guid_parse(char *uuidstr, unsigned char *uuid)
>+{
>+	int ret;
>+
>+	ret = uuid_parse(uuidstr, uuid);
>+	if (ret < 0)
>+		return ret;
>+
>+	if (__use_guid)
>+		convert_uuid_to_guid(uuid);
>+
>+	return ret;
>+}
>+
>+static int
>+fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
>+			  size_t idx, char *uuids)
>+{
>+	struct fwu_image_entry *image = fwu_get_image(mobj, idx);
>+	struct fwu_image_bank_info *bank;
>+	char *p = uuids, *uuid;
>+	int i;
>+
>+	if (!image)
>+		return -ENOENT;
>+
>+	/* Image location UUID */
>+	uuid = strsep(&p, ",");
>+	if (!uuid)
>+		return -EINVAL;
>+
>+	if (strcmp(uuid, "0") &&
>+	    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
>+		return -EINVAL;
>+
>+	/* Image type UUID */
>+	uuid = strsep(&p, ",");
>+	if (!uuid)
>+		return -EINVAL;
>+
>+	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)
>+		return -EINVAL;
>+
>+	/* Fill bank image-UUID */
>+	for (i = 0; i < mobj->banks; i++) {
>+		bank = fwu_get_bank(mobj, idx, i);
>+		if (!bank)
>+			return -ENOENT;
>+		bank->accepted = 1;
>+		uuid = strsep(&p, ",");
>+		if (!uuid)
>+			return -EINVAL;
>+
>+		if (strcmp(uuid, "0") &&
>+		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
>+			return -EINVAL;
>+	}
>+	return 0;
>+}
>+
>+/* Caller must ensure that @uuids[] has @mobj->images entries. */
>+static int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[])
>+{
>+	struct fwu_mdata *mdata = mobj->mdata;
>+	int i, ret;
>+
>+	mdata->version = FWU_MDATA_VERSION;
>+	mdata->active_index = active_bank;
>+	mdata->previous_active_index = previous_bank;
>+
>+	for (i = 0; i < mobj->images; i++) {
>+		ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
>+		if (ret < 0)
>+			return ret;
>+	}
>+
>+	mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,
>+			     mobj->size - sizeof(uint32_t));
>+
>+	return 0;
>+}
>+
>+static int
>+fwu_make_mdata(size_t images, size_t banks, char *uuids[], char *output)
>+{
>+	struct fwu_mdata_object *mobj;
>+	FILE *file;
>+	int ret;
>+
>+	mobj = fwu_alloc_mdata(images, banks);
>+	if (!mobj)
>+		return -ENOMEM;
>+
>+	ret = fwu_parse_fill_uuids(mobj, uuids);
>+	if (ret < 0)
>+		goto done_make;
>+
>+	file = fopen(output, "w");
>+	if (!file) {
>+		ret = -errno;
>+		goto done_make;
>+	}
>+
>+	ret = fwrite(mobj->mdata, mobj->size, 1, file);
>+	if (ret != mobj->size)
>+		ret = -errno;
>+	else
>+		ret = 0;
>+
>+	fclose(file);
>+
>+done_make:
>+	free(mobj->mdata);
>+	free(mobj);
>+
>+	return ret;
>+}
>+
>+int main(int argc, char *argv[])
>+{
>+	unsigned long banks = 0, images = 0;
>+	int c, ret;
>+
>+	/* Explicitly initialize defaults */
>+	active_bank = 0;
>+	__use_guid = false;
>+	previous_bank = INT_MAX;
>+
>+	do {
>+		c = getopt_long(argc, argv, opts_short, options, NULL);
>+		switch (c) {
>+		case 'h':
>+			print_usage();
>+			return 0;
>+		case 'b':
>+			banks = strtoul(optarg, NULL, 0);
>+			break;
>+		case 'i':
>+			images = strtoul(optarg, NULL, 0);
>+			break;
>+		case 'g':
>+			__use_guid = true;
>+			break;
>+		case 'p':
>+			previous_bank = strtoul(optarg, NULL, 0);
>+			break;
>+		case 'a':
>+			active_bank = strtoul(optarg, NULL, 0);
>+			break;
>+		}
>+	} while (c != -1);
>+
>+	if (!banks || !images) {
>+		fprintf(stderr, "Error: The number of banks and images must not be 0.\n");
>+		return -EINVAL;
>+	}
>+
>+	/* This command takes UUIDs * images and output file. */
>+	if (optind + images + 1 != argc) {
>+		fprintf(stderr, "Error: UUID list or output file is not specified or too much.\n");
>+		print_usage();
>+		return -ERANGE;
>+	}
>+
>+	if (previous_bank == INT_MAX) {
>+		/* set to the earlier bank in round-robin scheme */
>+		previous_bank = active_bank > 0 ? active_bank - 1 : banks - 1;
>+	}
>+
>+	ret = fwu_make_mdata(images, banks, argv + optind, argv[argc - 1]);
>+	if (ret < 0)
>+		fprintf(stderr, "Error: Failed to parse and write image: %s\n",
>+			strerror(-ret));
>+
>+	return ret;
>+}

Re: [PATCH v4 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[Michal Simek]

On 3/27/23 23:14, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Introduce support for mtd backed storage for FWU feature and enable it on
> Synquacer platform based DeveloperBox.
> 
> This revision is rebased onto patchset that trims the FWU api
>   https://lore.kernel.org/u-boot/20230306231747.1888513-1-jassisinghbrar@gmail.com/
> 
> Changes since v3:
> 	* Fix and Update documentation to also build optee for FWU FIP image.
> 	* Fixed checkpatch warnings
> 	* Made local functions static.
> 	* Split config changes to a separate patch
> 	* Fix authorship of three patches.
> 
> 
> Jassi Brar (3):
>    dt: fwu: developerbox: enable fwu banks and mdata regions
>    configs: move to new flash layout and boot flow
>    fwu: DeveloperBox: add support for FWU
> 
> Masami Hiramatsu (3):
>    FWU: Add FWU metadata access driver for MTD storage regions
>    FWU: mtd: Add helper functions for accessing FWU metadata
>    tools: Add mkfwumdata tool for FWU metadata image
> 
>   .../synquacer-sc2a11-developerbox-u-boot.dtsi |  49 ++-
>   board/socionext/developerbox/Makefile         |   1 +
>   board/socionext/developerbox/developerbox.c   |   8 +
>   board/socionext/developerbox/fwu_plat.c       |  57 +++
>   configs/synquacer_developerbox_defconfig      |  12 +-
>   doc/board/socionext/developerbox.rst          | 155 +++++++-
>   drivers/fwu-mdata/Kconfig                     |  15 +
>   drivers/fwu-mdata/Makefile                    |   1 +
>   drivers/fwu-mdata/raw_mtd.c                   | 272 ++++++++++++++
>   include/configs/synquacer.h                   |  10 +
>   include/fwu.h                                 |  34 ++
>   lib/fwu_updates/Makefile                      |   1 +
>   lib/fwu_updates/fwu_mtd.c                     | 164 +++++++++
>   tools/Kconfig                                 |   9 +
>   tools/Makefile                                |   4 +
>   tools/mkfwumdata.c                            | 334 ++++++++++++++++++
>   16 files changed, 1115 insertions(+), 11 deletions(-)
>   create mode 100644 board/socionext/developerbox/fwu_plat.c
>   create mode 100644 drivers/fwu-mdata/raw_mtd.c
>   create mode 100644 lib/fwu_updates/fwu_mtd.c
>   create mode 100644 tools/mkfwumdata.c
> 

I have played with this more and I found other things.

Ilias:
mkeficapsule is accepting only guid and mkfwumdata is accepting only uuid or 
guids. And DT is having uuids only.
I think it will be good to sync it up because you need to have both version for 
images generation which is a little bit painful. It should be possible to list 
only guids or uuids.

And it is not clear to me how u-boot is talking to boot firmware about next boot 
index. fwu_plat_get_bootidx() returns index which booted but I can't see any 
hoook to tell from u-boot to boot firmware what should be image/index to boot 
after capsule update.

I expect it should work like this.
Boot to u-boot - let's say index 0.
Run capsule update which will update index 1.
Inform boot firmware to boot index 1 (this step I am missing)
Call reset
Boot to u-boot index 1.
Apply accept capsule to confirm that image at index 1 is correct

in case of error (watchdog for example)
boot to u-boot index 0
apply revert capsule and disable index 1 image.

Thanks,
Michal

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:

> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 0000000000..4b1a10073a
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,272 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>
just c&p. though isn't that the same as GPL-2.0-or-later ?

.......
> > +
> > +extern struct fwu_mtd_image_info fwu_mtd_images[];
>
> if there is a need to share it. It should go to header.
>
include/fwu,h  would be the header. Which is supposed to be very
global, and doesn't seem very appropriate to mention private data
shared between a driver and helper library.
If people still insist, I will make the change.

> And fwu_mtd_images[] is not defined when only this patch is applied.
> It means order of patches is not right. 1/6 and 2/6 should be swapped
>
I think 1 and 2 should be merged rather.


......
> > +     if (!mtd_priv->mtd) {
> > +             log_err("Failed to find mtd device by fwu-mdata-store\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* Get the offset of primary and seconday mdata */
>
>
> typo
>
ok

......
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > +     { .compatible = "u-boot,fwu-mdata-mtd" },
> > +     { }
> > +};
>
> As I said this DT binding should be approved first to make sure that we don't
> need to fix DT binding in future. Just simply do it right from the begining.
>
Yes, I will cc Rob in the next submission (I only forgot last time).
However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
I am not sure if there is any reason for the fwu node to even be in
the dts for kernel. But sure it is good to have it eyeballed by the DT
gods.

cheers.

Re: [PATCH v4 2/6] FWU: mtd: Add helper functions for accessing FWU metadata

[Jassi Brar]

On Wed, 29 Mar 2023 at 06:55, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:


> > +/**
> > + * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
> > + * @buf: Buffer into which the dfu_alt_info is filled
> > + * @len: Maximum characters that can be written in buf
> > + * @mtd: Pointer to underlying MTD device
> > + *
> > + * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above.
>
ok

> > + */
> > +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
> > +
> > +/**
> > + * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
> > + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
>
> it doesn't match with parameter below.
>
ok

> > + * @alt_num: Pointer to the alt_num
> > + * @mtd_dev: Name of mtd device instance
> > + *
> > + * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above
>
ok

> > + */
> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char *mtd_dev);
>
>
> ./scripts/kernel-doc -v -man include/fwu.h 1>/dev/null
>
> include/fwu.h:276: info: Scanning doc for fwu_mtd_get_alt_num
> include/fwu.h:286: warning: Function parameter or member 'image_id' not
> described in 'fwu_mtd_get_alt_num'
> include/fwu.h:286: warning: Excess function parameter 'image_guid' description
> in 'fwu_mtd_get_alt_num'
>
ok


> > +
> > +static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
> > +{
> > +     int num_images = sizeof(fwu_mtd_images) / sizeof(struct fwu_mtd_image_info);
>
> include/linux/kernel.h:55:#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
of course. thanks.


> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
> > +                     const char *mtd_dev)
> > +{
> > +     int i, nalt;
> > +     int ret = -1;
>
> nit: can be on the same line.
>
ok

> > +     struct mtd_info *mtd;
> > +     struct dfu_entity *dfu;
> > +     fdt_addr_t offset, size = 0;
> > +     char uuidbuf[UUID_STR_LEN + 1];
> > +     struct fwu_mtd_image_info *mtd_img_info;
>
> nit: reverse christmas tree order
>
ok

> > +
> > +     mtd_probe_devices();
> > +     mtd = get_mtd_device_nm(mtd_dev);
>
> you are getting link but you are not using it anywhere.
> You should check return value or remove this call completely.
>
This should go. thanks.


> > +
> > +     uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
> > +
> > +     mtd_img_info = mtd_img_by_uuid(uuidbuf);
> > +     if (!mtd_img_info) {
> > +             log_err("%s: Not found partition for image %s\n", __func__, uuidbuf);
> > +             return -ENOENT;
> > +     }
>
> nit: newline
>
ok

> > +     offset = mtd_img_info->start;
> > +     size = mtd_img_info->size;
> > +
> > +     dfu_init_env_entities(NULL, NULL);
>
> worth to check return value here.
>
ok, though it would also cleanly fail immediately after this call when
it find empty dfu_list.

Thanks.

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:

> > diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> > new file mode 100644
> > index 0000000000..43dabf3b72
> > --- /dev/null
> > +++ b/tools/mkfwumdata.c
> > @@ -0,0 +1,334 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include <errno.h>
> > +#include <getopt.h>
> > +#include <limits.h>
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <u-boot/crc.h>
> > +#include <unistd.h>
> > +#include <uuid/uuid.h>
> > +
> > +/* This will dynamically allocate the fwu_mdata */
> > +#define CONFIG_FWU_NUM_BANKS         0
> > +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
>
> These two are completely unused.
>
these are necessary for include/fwu_mdata.h that comes later.

cheers.

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Thu, 30 Mar 2023 at 01:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 27. März 2023 23:16:10 MESZ schrieb jassisinghbrar@gmail.com:
> >From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >
> >Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> >partition to be used in A/B Update imeplementation.
> >
> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >---
> > tools/Kconfig      |   9 ++
> > tools/Makefile     |   4 +
> > tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 347 insertions(+)
> > create mode 100644 tools/mkfwumdata.c
>
> For each command line tool there should be a man-page.
>
ok

.....
> >+
> >+/* This will dynamically allocate the fwu_mdata */
> >+#define CONFIG_FWU_NUM_BANKS          0
> >+#define CONFIG_FWU_NUM_IMAGES_PER_BANK        0
> >+
> >+/* Since we can not include fwu.h, redefine version here. */
> >+#define FWU_MDATA_VERSION             1
> >+
> >+typedef uint8_t u8;
> >+typedef int16_t s16;
> >+typedef uint16_t u16;
> >+typedef uint32_t u32;
> >+typedef uint64_t u64;
> >+
> >+#include <fwu_mdata.h>
> >+
> >+/* TODO: Endianness conversion may be required for some arch. */
>
> Does the update work without UEFI? UEFI requires low endianness.
>
Not sure what you mean. This only creates a meta-data image that goes
into some offset in mtd.


> >+static void print_usage(void)
> >+{
> >+      fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\n");
> >+      fprintf(stderr, "Options:\n"
> >+              "\t-i, --images <num>          Number of images\n"
> >+              "\t-b, --banks  <num>          Number of banks\n"
> >+              "\t-a, --active-bank  <num>    Active bank\n"
> >+              "\t-p, --previous-bank  <num>  Previous active bank\n"
> >+              "\t-g, --guid                  Use GUID instead of UUID\n"
>
> I would not know what this means.
>
> Why can't you supply that single GUID in the position of the UUIDs parameter?
>
I guess the original authors wanted to have one explicitly specify
what's going on, rather than 'hacking' around (?)

thanks

Re: [PATCH v4 6/6] fwu: DeveloperBox: add support for FWU

[Jassi Brar]

On Wed, 29 Mar 2023 at 08:02, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:

.....
> > +
> > +void fwu_plat_get_bootidx(uint *boot_idx)
> > +{
> > +     int ret;
> > +     u32 active_idx;
> > +     u32 *bootidx = boot_idx;
> > +
> > +     ret = fwu_get_active_index(&active_idx);
> > +
>
> nit: remove this newline
>
ok

> > +     if (ret < 0)
> > +             *bootidx = -1;
> > +
> > +     *bootidx = active_idx;
>
> Is this logic here right?
> If fwu_get_active_index fails you setup bootidx to -1
> and right after it you rewrite it to active_idx initialized in
> fwu_get_active_index() to mdata->active_index.
>
> It means why to do *bootidx = -1; at all?
>
yes :) it's a silly remnant of history of changes.
Actually this goes away after implementing the default/weak function.


> > +}
> > diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
> > index 09e12b739b..d09684153a 100644
> > --- a/configs/synquacer_developerbox_defconfig
> > +++ b/configs/synquacer_developerbox_defconfig
> > @@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> >   CONFIG_EFI_CAPSULE_ON_DISK=y
> >   CONFIG_EFI_IGNORE_OSINDICATIONS=y
> >   CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > +CONFIG_EFI_SECURE_BOOT=y
> > +CONFIG_FWU_MULTI_BANK_UPDATE=y
> > +CONFIG_FWU_MDATA=y
> > +CONFIG_FWU_MDATA_MTD=y
> > +CONFIG_FWU_NUM_BANKS=2
> > +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> > +CONFIG_CMD_FWU_METADATA=y
> > +CONFIG_TOOLS_MKFWUMDATA=y
>
> doesn't look like that it was created via savedefconfig.
>
Yes. I had some other config changes too and picked only the relevant
ones together.


> > +And make a FIP image.::
> > +
> > +  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
> > +  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
> > +
> > +UUIDs for the FWU Multi Bank Update
> > +-----------------------------------
> > +
> > +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
> > +following UUIDs.
> > +
> > + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
>
>
> In past you have it listed at flash node in DT. I see you have removed it
> between v3 and v4 without any note about it.
> Is it still needed? And should it be listed in DT spec again?
>
After the dt change, we no longer require this. But the location_uuid
is a standard member of an fwu_image_entry and cmd/fwu_mdata.c always
print it. So I think this should be seen as just what a platform wants
some unique id to be printed for the image (?).

Thanks Michal for reviewing v4.
cheers.

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
> >
> > From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >
> > Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> > partition to be used in A/B Update imeplementation.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >  tools/Kconfig      |   9 ++
> >  tools/Makefile     |   4 +
> >  tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 347 insertions(+)
> >  create mode 100644 tools/mkfwumdata.c
>
> Can you please look at putting this in binman instead, since we would
> rather not have another tool with no tests.
>
Must I do that? I have no history with binman and it seems the
mkfwumdata.c would need to be rewritten in python?

regards.

Re: [PATCH v4 6/6] fwu: DeveloperBox: add support for FWU

[Michal Simek]

On 4/10/23 06:21, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 08:02, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> 
> .....
>>> +
>>> +void fwu_plat_get_bootidx(uint *boot_idx)
>>> +{
>>> +     int ret;
>>> +     u32 active_idx;
>>> +     u32 *bootidx = boot_idx;
>>> +
>>> +     ret = fwu_get_active_index(&active_idx);
>>> +
>>
>> nit: remove this newline
>>
> ok
> 
>>> +     if (ret < 0)
>>> +             *bootidx = -1;
>>> +
>>> +     *bootidx = active_idx;
>>
>> Is this logic here right?
>> If fwu_get_active_index fails you setup bootidx to -1
>> and right after it you rewrite it to active_idx initialized in
>> fwu_get_active_index() to mdata->active_index.
>>
>> It means why to do *bootidx = -1; at all?
>>
> yes :) it's a silly remnant of history of changes.
> Actually this goes away after implementing the default/weak function.
> 
> 
>>> +}
>>> diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
>>> index 09e12b739b..d09684153a 100644
>>> --- a/configs/synquacer_developerbox_defconfig
>>> +++ b/configs/synquacer_developerbox_defconfig
>>> @@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>>>    CONFIG_EFI_CAPSULE_ON_DISK=y
>>>    CONFIG_EFI_IGNORE_OSINDICATIONS=y
>>>    CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>> +CONFIG_EFI_SECURE_BOOT=y
>>> +CONFIG_FWU_MULTI_BANK_UPDATE=y
>>> +CONFIG_FWU_MDATA=y
>>> +CONFIG_FWU_MDATA_MTD=y
>>> +CONFIG_FWU_NUM_BANKS=2
>>> +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
>>> +CONFIG_CMD_FWU_METADATA=y
>>> +CONFIG_TOOLS_MKFWUMDATA=y
>>
>> doesn't look like that it was created via savedefconfig.
>>
> Yes. I had some other config changes too and picked only the relevant
> ones together.

But this is defconfig not documentation.



> 
> 
>>> +And make a FIP image.::
>>> +
>>> +  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
>>> +  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
>>> +
>>> +UUIDs for the FWU Multi Bank Update
>>> +-----------------------------------
>>> +
>>> +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
>>> +following UUIDs.
>>> +
>>> + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
>>
>>
>> In past you have it listed at flash node in DT. I see you have removed it
>> between v3 and v4 without any note about it.
>> Is it still needed? And should it be listed in DT spec again?
>>
> After the dt change, we no longer require this. But the location_uuid
> is a standard member of an fwu_image_entry and cmd/fwu_mdata.c always
> print it. So I think this should be seen as just what a platform wants
> some unique id to be printed for the image (?).

I am fine with your explanation but documentation should make this clear that 
uuid is required by spec but not actually used by current implementation or 
description.

M

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 4/10/23 06:25, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi,
>>
>> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
>>>
>>> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>>
>>> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
>>> partition to be used in A/B Update imeplementation.
>>>
>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>> ---
>>>   tools/Kconfig      |   9 ++
>>>   tools/Makefile     |   4 +
>>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 347 insertions(+)
>>>   create mode 100644 tools/mkfwumdata.c
>>
>> Can you please look at putting this in binman instead, since we would
>> rather not have another tool with no tests.
>>
> Must I do that? I have no history with binman and it seems the
> mkfwumdata.c would need to be rewritten in python?

I think it is about calling this utility from python not about rewriting it to 
python.

M

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 4/10/23 06:05, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> 
>>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>>> new file mode 100644
>>> index 0000000000..43dabf3b72
>>> --- /dev/null
>>> +++ b/tools/mkfwumdata.c
>>> @@ -0,0 +1,334 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2023, Linaro Limited
>>> + */
>>> +
>>> +#include <errno.h>
>>> +#include <getopt.h>
>>> +#include <limits.h>
>>> +#include <stdio.h>
>>> +#include <stdint.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +#include <u-boot/crc.h>
>>> +#include <unistd.h>
>>> +#include <uuid/uuid.h>
>>> +
>>> +/* This will dynamically allocate the fwu_mdata */
>>> +#define CONFIG_FWU_NUM_BANKS         0
>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
>>
>> These two are completely unused.
>>
> these are necessary for include/fwu_mdata.h that comes later.

If that's come later, add it later.

M

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Michal Simek]

On 4/10/23 05:56, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> 
>>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
>>> new file mode 100644
>>> index 0000000000..4b1a10073a
>>> --- /dev/null
>>> +++ b/drivers/fwu-mdata/raw_mtd.c
>>> @@ -0,0 +1,272 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>
>> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>>
> just c&p. though isn't that the same as GPL-2.0-or-later ?

license choice is up to you. We normally use just gpl-2.0.

> 
> .......
>>> +
>>> +extern struct fwu_mtd_image_info fwu_mtd_images[];
>>
>> if there is a need to share it. It should go to header.
>>
> include/fwu,h  would be the header. Which is supposed to be very
> global, and doesn't seem very appropriate to mention private data
> shared between a driver and helper library.
> If people still insist, I will make the change.
> 
>> And fwu_mtd_images[] is not defined when only this patch is applied.
>> It means order of patches is not right. 1/6 and 2/6 should be swapped
>>
> I think 1 and 2 should be merged rather.

no issue with it.

> 
> ......
>>> +     if (!mtd_priv->mtd) {
>>> +             log_err("Failed to find mtd device by fwu-mdata-store\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     /* Get the offset of primary and seconday mdata */
>>
>>
>> typo
>>
> ok
> 
> ......
>>> +
>>> +static const struct udevice_id fwu_mdata_ids[] = {
>>> +     { .compatible = "u-boot,fwu-mdata-mtd" },
>>> +     { }
>>> +};
>>
>> As I said this DT binding should be approved first to make sure that we don't
>> need to fix DT binding in future. Just simply do it right from the begining.
>>
> Yes, I will cc Rob in the next submission (I only forgot last time).
> However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> I am not sure if there is any reason for the fwu node to even be in
> the dts for kernel. But sure it is good to have it eyeballed by the DT
> gods.

It doesn't really go to kernel.
Simon pushed options node directly to dt-schema
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
And that would be also location for this node too.

Thanks,
Michal

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Fri, Apr 14, 2023 at 8:53 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/10/23 06:05, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
> >> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> >
> >>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> >>> new file mode 100644
> >>> index 0000000000..43dabf3b72
> >>> --- /dev/null
> >>> +++ b/tools/mkfwumdata.c
> >>> @@ -0,0 +1,334 @@
> >>> +// SPDX-License-Identifier: GPL-2.0+
> >>> +/*
> >>> + * Copyright (c) 2023, Linaro Limited
> >>> + */
> >>> +
> >>> +#include <errno.h>
> >>> +#include <getopt.h>
> >>> +#include <limits.h>
> >>> +#include <stdio.h>
> >>> +#include <stdint.h>
> >>> +#include <stdlib.h>
> >>> +#include <string.h>
> >>> +#include <u-boot/crc.h>
> >>> +#include <unistd.h>
> >>> +#include <uuid/uuid.h>
> >>> +
> >>> +/* This will dynamically allocate the fwu_mdata */
> >>> +#define CONFIG_FWU_NUM_BANKS         0
> >>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>
> >> These two are completely unused.
> >>
> > these are necessary for include/fwu_mdata.h that comes later.
>
> If that's come later, add it later.
>
Can you please actually look at the code to see the underlying constraint?

-j

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

On Fri, Apr 14, 2023 at 8:56 AM Michal Simek <michal.simek@amd.com> wrote:
> On 4/10/23 05:56, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> >> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> >
> >>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> >>> new file mode 100644
> >>> index 0000000000..4b1a10073a
> >>> --- /dev/null
> >>> +++ b/drivers/fwu-mdata/raw_mtd.c
> >>> @@ -0,0 +1,272 @@
> >>> +// SPDX-License-Identifier: GPL-2.0+
> >>
> >> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
> >>
> > just c&p. though isn't that the same as GPL-2.0-or-later ?
>
> license choice is up to you. We normally use just gpl-2.0.
>
I think more than "we", the subsystem dictates licensing. All FWU code
is under GPL-2.0-or-later.


> >>
> >> As I said this DT binding should be approved first to make sure that we don't
> >> need to fix DT binding in future. Just simply do it right from the begining.
> >>
> > Yes, I will cc Rob in the next submission (I only forgot last time).
> > However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> > I am not sure if there is any reason for the fwu node to even be in
> > the dts for kernel. But sure it is good to have it eyeballed by the DT
> > gods.
>
> It doesn't really go to kernel.
> Simon pushed options node directly to dt-schema
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
> And that would be also location for this node too.
>
Yes, but I have resend the already existisng bindings in uboot. My
patch only modified them. Not a big problem.

-j

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 4/14/23 17:02, Jassi Brar wrote:
> On Fri, Apr 14, 2023 at 8:53 AM Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 4/10/23 06:05, Jassi Brar wrote:
>>> On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
>>>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
>>>
>>>>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>>>>> new file mode 100644
>>>>> index 0000000000..43dabf3b72
>>>>> --- /dev/null
>>>>> +++ b/tools/mkfwumdata.c
>>>>> @@ -0,0 +1,334 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * Copyright (c) 2023, Linaro Limited
>>>>> + */
>>>>> +
>>>>> +#include <errno.h>
>>>>> +#include <getopt.h>
>>>>> +#include <limits.h>
>>>>> +#include <stdio.h>
>>>>> +#include <stdint.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <string.h>
>>>>> +#include <u-boot/crc.h>
>>>>> +#include <unistd.h>
>>>>> +#include <uuid/uuid.h>
>>>>> +
>>>>> +/* This will dynamically allocate the fwu_mdata */
>>>>> +#define CONFIG_FWU_NUM_BANKS         0
>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
>>>>
>>>> These two are completely unused.
>>>>
>>> these are necessary for include/fwu_mdata.h that comes later.
>>
>> If that's come later, add it later.
>>
> Can you please actually look at the code to see the underlying constraint?

Ok. I misunderstand your comment.

You have it there because of #include <fwu_mdata.h> and using macros there.
Can you just allocated that structures at run time instead of statically defined 
them via array? That would clean that design and also fix checkpatch issue.

Thanks,
Michal

$ ./scripts/checkpatch.pl -f tools/mkfwumdata.c
ERROR: All CONFIG symbols are managed by Kconfig
#18: FILE: tools/mkfwumdata.c:18:
+#define CONFIG_FWU_NUM_BANKS		0

ERROR: All CONFIG symbols are managed by Kconfig
#19: FILE: tools/mkfwumdata.c:19:
+#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0

total: 2 errors, 0 warnings, 0 checks, 334 lines checked

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
> On 4/14/23 17:02, Jassi Brar wrote:

> >>>>> +
> >>>>> +/* This will dynamically allocate the fwu_mdata */
> >>>>> +#define CONFIG_FWU_NUM_BANKS         0
> >>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>>>
> >>>> These two are completely unused.
> >>>>
> >>> these are necessary for include/fwu_mdata.h that comes later.
> >>
> >> If that's come later, add it later.
> >>
> > Can you please actually look at the code to see the underlying constraint?
>
> Ok. I misunderstand your comment.
>
> You have it there because of #include <fwu_mdata.h> and using macros there.
> Can you just allocated that structures at run time instead of statically defined
> them via array? That would clean that design and also fix checkpatch issue.
>
I can't see how dynamically allocating an array of packed structures
inside an array of packed structures , makes the design any cleaner.

I think this is a valid case where we can ignore the checkpatch
warning because we know what we are doing.

-jassi

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 4/17/23 15:48, Jassi Brar wrote:
> On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
>> On 4/14/23 17:02, Jassi Brar wrote:
> 
>>>>>>> +
>>>>>>> +/* This will dynamically allocate the fwu_mdata */
>>>>>>> +#define CONFIG_FWU_NUM_BANKS         0
>>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
>>>>>>
>>>>>> These two are completely unused.
>>>>>>
>>>>> these are necessary for include/fwu_mdata.h that comes later.
>>>>
>>>> If that's come later, add it later.
>>>>
>>> Can you please actually look at the code to see the underlying constraint?
>>
>> Ok. I misunderstand your comment.
>>
>> You have it there because of #include <fwu_mdata.h> and using macros there.
>> Can you just allocated that structures at run time instead of statically defined
>> them via array? That would clean that design and also fix checkpatch issue.
>>
> I can't see how dynamically allocating an array of packed structures
> inside an array of packed structures , makes the design any cleaner.

It is not marked as __packed and based on what you are saying it should be 
marked like that.

> 
> I think this is a valid case where we can ignore the checkpatch
> warning because we know what we are doing.

I will let maintainer, who will merge this code, to decide on this.
I would at least make that comment much bigger to explain it better
that you are doing it because you don't want to redefine structures from 
fwu_mdata.h.

Thanks,
Michal

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Mon, 17 Apr 2023 at 09:29, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/17/23 15:48, Jassi Brar wrote:
> > On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
> >> On 4/14/23 17:02, Jassi Brar wrote:
> >
> >>>>>>> +
> >>>>>>> +/* This will dynamically allocate the fwu_mdata */
> >>>>>>> +#define CONFIG_FWU_NUM_BANKS         0
> >>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>>>>>
> >>>>>> These two are completely unused.
> >>>>>>
> >>>>> these are necessary for include/fwu_mdata.h that comes later.
> >>>>
> >>>> If that's come later, add it later.
> >>>>
> >>> Can you please actually look at the code to see the underlying constraint?
> >>
> >> Ok. I misunderstand your comment.
> >>
> >> You have it there because of #include <fwu_mdata.h> and using macros there.
> >> Can you just allocated that structures at run time instead of statically defined
> >> them via array? That would clean that design and also fix checkpatch issue.
> >>
> > I can't see how dynamically allocating an array of packed structures
> > inside an array of packed structures , makes the design any cleaner.
>
> It is not marked as __packed and based on what you are saying it should be
> marked like that.
>
Those structures already existed before my patchset. And I definitely
remember they were suggested to be marked as packed but were not. So
now you have to read to code to understand what they are.

> >
> > I think this is a valid case where we can ignore the checkpatch
> > warning because we know what we are doing.
>
> I will let maintainer, who will merge this code, to decide on this.
> I would at least make that comment much bigger to explain it better
> that you are doing it because you don't want to redefine structures from
> fwu_mdata.h.
>
you mean redeclare, right?  That is what the two define's do effectively.

-j

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Simon Glass]

Hi,

On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/10/23 06:25, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
> >>>
> >>> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >>>
> >>> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> >>> partition to be used in A/B Update imeplementation.
> >>>
> >>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >>> ---
> >>>   tools/Kconfig      |   9 ++
> >>>   tools/Makefile     |   4 +
> >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 347 insertions(+)
> >>>   create mode 100644 tools/mkfwumdata.c
> >>
> >> Can you please look at putting this in binman instead, since we would
> >> rather not have another tool with no tests.
> >>
> > Must I do that? I have no history with binman and it seems the
> > mkfwumdata.c would need to be rewritten in python?
>
> I think it is about calling this utility from python not about rewriting it to
> python.

Yes that's the question. If this tool is for creating firmware
updates, then how are they created? I would expect binman to handle
this when U-Boot is built. Then you can build in some tests in binman
perhaps?

How does one know what parameters to pass? Is the documentation for
this tool elsewhere? Where are the tests?

It is also unfortunate that this seems to be inventing yet another
format (I recall that FIP was invented at one point also), when it
could use FIT.

Regards,
Simon

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Tue, 18 Apr 2023 at 20:46, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
> >
> >
> >
> > On 4/10/23 06:25, Jassi Brar wrote:
> > > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
> > >>>
> > >>> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >>>
> > >>> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> > >>> partition to be used in A/B Update imeplementation.
> > >>>
> > >>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > >>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > >>> ---
> > >>>   tools/Kconfig      |   9 ++
> > >>>   tools/Makefile     |   4 +
> > >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > >>>   3 files changed, 347 insertions(+)
> > >>>   create mode 100644 tools/mkfwumdata.c
> > >>
> > >> Can you please look at putting this in binman instead, since we would
> > >> rather not have another tool with no tests.
> > >>
> > > Must I do that? I have no history with binman and it seems the
> > > mkfwumdata.c would need to be rewritten in python?
> >
> > I think it is about calling this utility from python not about rewriting it to
> > python.
>
> Yes that's the question. If this tool is for creating firmware
> updates, then how are they created? I would expect binman to handle
> this when U-Boot is built. Then you can build in some tests in binman
> perhaps?
>
The FWU meta-data format is specified in a standards document created
by ARM and not tied to u-boot.
U-Boot may not necessarily be the bootloader using the output of
mkfwumdata. The u-boot/tools/ is more like a welcoming host.

Ideally mkfwumdata should be a reference implementation, also created
by ARM. But it is trivial enough that nobody thought there could be
any confusion about the format, I guess.

> How does one know what parameters to pass? Is the documentation for
> this tool elsewhere?
>
In the latest submission I also created a man-page for it.

>  Where are the tests?
>
I am open to learning what could be tested and how.

> It is also unfortunate that this seems to be inventing yet another
> format (I recall that FIP was invented at one point also), when it
> could use FIT.
>
Hopefully it won't be that bad of a predicament after my explanation above.

cheers.

Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Simon Glass]

Hi Jassi,

On Tue, 18 Apr 2023 at 20:58, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Tue, 18 Apr 2023 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > >
> > >
> > > On 4/10/23 06:25, Jassi Brar wrote:
> > > > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> wrote:
> > > >>>
> > > >>> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >>>
> > > >>> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> > > >>> partition to be used in A/B Update imeplementation.
> > > >>>
> > > >>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > >>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > >>> ---
> > > >>>   tools/Kconfig      |   9 ++
> > > >>>   tools/Makefile     |   4 +
> > > >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > > >>>   3 files changed, 347 insertions(+)
> > > >>>   create mode 100644 tools/mkfwumdata.c
> > > >>
> > > >> Can you please look at putting this in binman instead, since we would
> > > >> rather not have another tool with no tests.
> > > >>
> > > > Must I do that? I have no history with binman and it seems the
> > > > mkfwumdata.c would need to be rewritten in python?
> > >
> > > I think it is about calling this utility from python not about rewriting it to
> > > python.
> >
> > Yes that's the question. If this tool is for creating firmware
> > updates, then how are they created? I would expect binman to handle
> > this when U-Boot is built. Then you can build in some tests in binman
> > perhaps?
> >
> The FWU meta-data format is specified in a standards document created
> by ARM and not tied to u-boot.
> U-Boot may not necessarily be the bootloader using the output of
> mkfwumdata. The u-boot/tools/ is more like a welcoming host.
>
> Ideally mkfwumdata should be a reference implementation, also created
> by ARM. But it is trivial enough that nobody thought there could be
> any confusion about the format, I guess.

OK

>
> > How does one know what parameters to pass? Is the documentation for
> > this tool elsewhere?
> >
> In the latest submission I also created a man-page for it.

Good

>
> >  Where are the tests?
> >
> I am open to learning what could be tested and how.

Well normally we would have a binman test which uses the tool to
create an image, then checks that it works. See ftest.py for some
examples.

>
> > It is also unfortunate that this seems to be inventing yet another
> > format (I recall that FIP was invented at one point also), when it
> > could use FIT.
> >
> Hopefully it won't be that bad of a predicament after my explanation above.

Regards,
Simon

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Ilias Apalodimas]

HI Jassi, Michal

Based on the discussion we had on the dt bindings, I am personally ok with
the notion of having those defined internally until we can prove it makes
sense for the to be sent to the dt-schema.

In the future we need to strip those from U-Boot, before we hand over the
DR to the OS, but this is a problem that already exists regardless of this patchset.

Jassi, Michal had some review comments.  Are you going to send a v5 with
those fixed?

Thanks
/Ilias

On Fri, Apr 14, 2023 at 10:09:11AM -0500, Jassi Brar wrote:
> On Fri, Apr 14, 2023 at 8:56 AM Michal Simek <michal.simek@amd.com> wrote:
> > On 4/10/23 05:56, Jassi Brar wrote:
> > > On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> > >> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> > >
> > >>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > >>> new file mode 100644
> > >>> index 0000000000..4b1a10073a
> > >>> --- /dev/null
> > >>> +++ b/drivers/fwu-mdata/raw_mtd.c
> > >>> @@ -0,0 +1,272 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0+
> > >>
> > >> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
> > >>
> > > just c&p. though isn't that the same as GPL-2.0-or-later ?
> >
> > license choice is up to you. We normally use just gpl-2.0.
> >
> I think more than "we", the subsystem dictates licensing. All FWU code
> is under GPL-2.0-or-later.
> 
> 
> > >>
> > >> As I said this DT binding should be approved first to make sure that we don't
> > >> need to fix DT binding in future. Just simply do it right from the begining.
> > >>
> > > Yes, I will cc Rob in the next submission (I only forgot last time).
> > > However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> > > I am not sure if there is any reason for the fwu node to even be in
> > > the dts for kernel. But sure it is good to have it eyeballed by the DT
> > > gods.
> >
> > It doesn't really go to kernel.
> > Simon pushed options node directly to dt-schema
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
> > And that would be also location for this node too.
> >
> Yes, but I have resend the already existisng bindings in uboot. My
> patch only modified them. Not a big problem.
> 
> -j

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Michal Simek]

Hi Ilias,

On 5/19/23 14:21, Ilias Apalodimas wrote:
> HI Jassi, Michal
> 
> Based on the discussion we had on the dt bindings, I am personally ok with
> the notion of having those defined internally until we can prove it makes
> sense for the to be sent to the dt-schema.
> 
> In the future we need to strip those from U-Boot, before we hand over the
> DR to the OS, but this is a problem that already exists regardless of this patchset.
> 
> Jassi, Michal had some review comments.  Are you going to send a v5 with
> those fixed?

We created this patch for dt-schema and if there is no comment I will be sending 
it to Rob to get merge.

lore.kernel.org/r/ca0715934133dbce6a5a3fd91483e0af92ea8ac6.1683815597.git.michal.simek@amd.com

As I said I am fine with having it in u-boot tree only but that node has to be 
removed to pass certification.
It means I would expect that we will solve it together not later on because it 
will catch us very quickly.
If you have code in EFI to do that it, let's just do it together to be able to 
pass IR2.0 requirements without waiting for another patch.

Thanks,
Michal

Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Fri, May 19, 2023 at 02:45:26PM +0200, Michal Simek wrote:
> Hi Ilias,
> 
> On 5/19/23 14:21, Ilias Apalodimas wrote:
> > HI Jassi, Michal
> > 
> > Based on the discussion we had on the dt bindings, I am personally ok with
> > the notion of having those defined internally until we can prove it makes
> > sense for the to be sent to the dt-schema.
> > 
> > In the future we need to strip those from U-Boot, before we hand over the
> > DR to the OS, but this is a problem that already exists regardless of this patchset.
> > 
> > Jassi, Michal had some review comments.  Are you going to send a v5 with
> > those fixed?
> 
> We created this patch for dt-schema and if there is no comment I will be
> sending it to Rob to get merge.
> 
> lore.kernel.org/r/ca0715934133dbce6a5a3fd91483e0af92ea8ac6.1683815597.git.michal.simek@amd.com
> 
> As I said I am fine with having it in u-boot tree only but that node has to
> be removed to pass certification.
> It means I would expect that we will solve it together not later on because
> it will catch us very quickly.
> If you have code in EFI to do that it, let's just do it together to be able
> to pass IR2.0 requirements without waiting for another patch.

So I think in sum, I think the path today is to see if Rob will accept
that patch and if not, either rework based on feedback or if the
feedback is "this is U-Boot centric atm, not appropriate" we'll go with
the path of removing nodes before the OS gets the tree, which I think is
a generic enough bit of framework that we'll need regardless, for 2.0.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]