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

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...
Changes since v5: * Some of the typo fixes and cosmetic changes suggested by Etienne
Changes since v4: * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx * Provide man page for mkfwumdata * Misc typo fixes and cosmetic changes
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 (4): dt: fwu: developerbox: enable fwu banks and mdata regions configs: move to new flash layout and boot flow fwu: DeveloperBox: add support for FWU fwu: provide default fwu_plat_get_bootidx
Masami Hiramatsu (2): FWU: Add FWU metadata access driver for MTD storage regions 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 | 37 ++ configs/synquacer_developerbox_defconfig | 12 +- doc/board/socionext/developerbox.rst | 154 +++++++- doc/mkfwumdata.1 | 89 +++++ drivers/fwu-mdata/Kconfig | 15 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/raw_mtd.c | 269 ++++++++++++++ include/configs/synquacer.h | 10 + include/fwu.h | 32 ++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu.c | 18 + lib/fwu_updates/fwu_mtd.c | 185 ++++++++++ tools/Kconfig | 9 + tools/Makefile | 4 + tools/mkfwumdata.c | 334 ++++++++++++++++++ 18 files changed, 1217 insertions(+), 11 deletions(-) create mode 100644 board/socionext/developerbox/fwu_plat.c create mode 100644 doc/mkfwumdata.1 create mode 100644 drivers/fwu-mdata/raw_mtd.c create mode 100644 lib/fwu_updates/fwu_mtd.c create mode 100644 tools/mkfwumdata.c

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. The code is divided into core under drivers/fwu-mdata/ and some helper functions clubbed together under lib/fwu_updates/
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 | 269 ++++++++++++++++++++++++++++++++++++ include/fwu.h | 32 +++++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu_mtd.c | 185 +++++++++++++++++++++++++ 6 files changed, 503 insertions(+) create mode 100644 drivers/fwu-mdata/raw_mtd.c create mode 100644 lib/fwu_updates/fwu_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..17e4517973 --- /dev/null +++ b/drivers/fwu-mdata/raw_mtd.c @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * 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_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; + } + + /* This will expand erase size to align with the block size */ + lock_len = round_up(size, mtd->erasesize); + + ret = mtd_unlock(mtd, 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 = 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.datbuf = buf; + + 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, 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 secondary mdata */ + ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0, &label); + if (ret) + return ret; + strncpy(mtd_priv->pri_label, label, 50); + + 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; + strncpy(mtd_priv->sec_label, label, 50); + + 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 more images 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), +}; diff --git a/include/fwu.h b/include/fwu.h index 33b4d0b3db..edb7e9da90 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,28 @@ 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_guid, 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..b73111ae24 --- /dev/null +++ b/lib/fwu_updates/fwu_mtd.c @@ -0,0 +1,185 @@ +// 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 = ARRAY_SIZE(fwu_mtd_images); + + 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) +{ + struct fwu_mtd_image_info *mtd_img_info; + char uuidbuf[UUID_STR_LEN + 1]; + fdt_addr_t offset, size = 0; + struct dfu_entity *dfu; + int i, nalt, ret; + + mtd_probe_devices(); + + 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; + + ret = dfu_init_env_entities(NULL, NULL); + if (ret) + return -ENOENT; + + 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; + } + + ret = -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; +} + +/** + * fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the platform + * @dev: FWU device + * @image_id: Image GUID for which DFU alt number needs to be retrieved + * @alt_num: Pointer to the alt_num + * + * Get the DFU alt number from the platform for the image specified by the + * image GUID. + * + * Note: This is a weak function and platforms can override this with + * their own implementation for obtaining the alt number value. + * + * Return: 0 if OK, -ve on error + */ +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_id, + u8 *alt_num) +{ + return fwu_mtd_get_alt_num(image_id, alt_num, "nor1"); +} + +static int gen_image_alt_info(char *buf, size_t len, int sidx, + struct fwu_image_entry *img, struct mtd_info *mtd) +{ + char *p = buf, *end = buf + len; + int i; + + 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; +}

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: Sughosh Ganu sughosh.ganu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- doc/mkfwumdata.1 | 89 ++++++++++++ tools/Kconfig | 9 ++ tools/Makefile | 4 + tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 436 insertions(+) create mode 100644 doc/mkfwumdata.1 create mode 100644 tools/mkfwumdata.c
diff --git a/doc/mkfwumdata.1 b/doc/mkfwumdata.1 new file mode 100644 index 0000000000..7dd718b26e --- /dev/null +++ b/doc/mkfwumdata.1 @@ -0,0 +1,89 @@ +." SPDX-License-Identifier: GPL-2.0-or-later +." Copyright (C) 2023 Jassi Brar jaswinder.singh@linaro.org +.TH MKFWUMDATA 1 2023-04-10 U-Boot +.SH NAME +mkfwumdata - create FWU metadata image +. +.SH SYNOPSIS +.SY mkfwumdata +.OP -a activeidx +.OP -p previousidx +.OP -g +.BI -i~ imagecount +.BI -b~ bankcount +.I UUIDs +.I outputimage +.YS +.SY mkfwumdata +.B -h +.YS +. +.SH DESCRIPTION +.B mkfwumdata +creates metadata info to be used with FWU. +. +.SH OPTIONS +.TP +.B -h +Print usage information and exit. +. +.TP +.B -a +Set +.IR activeidx +as the currently active Bank. Default is 0. +. +.TP +.B -p +Set +.IR previousidx +as the previous active Bank. Default is +.IR activeidx "-1" +or +.IR bankcount "-1," +whichever is non-negative. +. +.TP +.B -g +Convert the +.IR UUIDs +as GUIDs before use. +. +.TP +.B -i +Specify there are +.IR imagecount +images in each bank. +. +.TP +.B -b +Specify there are a total of +.IR bankcount +banks. +. +.TP +.IR UUIDs +Comma-separated list of UUIDs required to create the metadata :- +location_uuid,image_type_uuid,<images per bank uuid list of all banks> +. +.TP +.IR outputimage +Specify the name of the metadata image file to be created. +. +.SH BUGS +Please report bugs to the +.UR https://%5C:source%5C:.denx%5C:.de/%5C:u-boot/%5C:u-boot/%5C:issues +U-Boot bug tracker +.UE . +.SH EXAMPLES +Create a metadata image with 2 banks and 1 image/bank, BankAct=0, BankPrev=1: +.PP +.EX +.in +4 +$ \c +.B mkfwumdata -a 0 -p 1 -b 2 -i 1 \\& +.in +6 +.B 17e86d77-41f9-4fd7-87ec-a55df9842de5,\\& +.B 10c36d7d-ca52-b843-b7b9-f9d6c501d108,\\& +.B 5a66a702-99fd-4fef-a392-c26e261a2828,a8f868a1-6e5c-4757-878d-ce63375ef2c0 \\& +.B fwu-mdata.img 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 d793cf3bec..a0cd87ff93 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -251,6 +251,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..9732a8ddc5 --- /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 (mandatory)\n" + "\t-b, --banks <num> Number of banks (mandatory)\n" + "\t-a, --active-bank <num> Active bank (default=0)\n" + "\t-p, --previous-bank <num> Previous active bank (default=active_bank - 1)\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; +}

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"; + }; + }; + }; }; };

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.
Acked-by: Etienne Carriere etienne.carriere@linaro.org 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 68f7bacf02..0c37897c9a 100644 --- a/configs/synquacer_developerbox_defconfig +++ b/configs/synquacer_developerbox_defconfig @@ -1,13 +1,13 @@ 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_SF_DEFAULT_SPEED=31250000 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"

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 | 37 +++++ configs/synquacer_developerbox_defconfig | 8 + doc/board/socionext/developerbox.rst | 154 +++++++++++++++++++- include/configs/synquacer.h | 10 ++ 6 files changed, 212 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..e724e702bd --- /dev/null +++ b/board/socionext/developerbox/fwu_plat.c @@ -0,0 +1,37 @@ +// 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) +{ + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); + struct mtd_info *mtd; + int ret; + + 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); +} diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig index 0c37897c9a..8e7236b572 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..aa7080e26c 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 the 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 details*
https://www.96boards.org/documentation/enterprise/developerbox/installation/...
-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 it will show the following boot +messages printed to the LS-UART0 console::
/*------------------------------------------*/ @@ -81,7 +87,143 @@ 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 board the +configs/synquacer_developerbox_defconfig enables default FWU configuration :: + + 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. + +Once the flasher tool is running we are ready to 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, \

From: Jassi Brar jaswinder.singh@linaro.org
Just like fwu_plat_get_update_index, provide a default/weak implementation of fwu_plat_get_bootidx. So that most platforms wouldn't have to re-implement the likely case.
Signed-off-by: Jassi Brar jaswinder.singh@linaro.org --- lib/fwu_updates/fwu.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index a24ccf567a..c9996f4d6d 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -545,6 +545,24 @@ __weak int fwu_plat_get_update_index(uint *update_idx) return ret; }
+/** + * fwu_plat_get_bootidx() - Get the value of the boot index + * @boot_idx: Boot index value + * + * Get the value of the bank(partition) from which the platform + * has booted. This value is passed to U-Boot from the earlier + * stage bootloader which loads and boots all the relevant + * firmware images + */ +__weak void fwu_plat_get_bootidx(uint *boot_idx) +{ + int ret; + + ret = fwu_get_active_index(boot_idx); + if (ret < 0) + *boot_idx = 0; /* Dummy value */ +} + /** * fwu_update_checks_pass() - Check if FWU update can be done *

On Wed, 31 May 2023 00:28:04 -0500, jaswinder.singh@linaro.org 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...
[...]
Applied to u-boot/next, thanks!

Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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...
Changes since v5: * Some of the typo fixes and cosmetic changes suggested by Etienne
Changes since v4: * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx * Provide man page for mkfwumdata * Misc typo fixes and cosmetic changes
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 (4): dt: fwu: developerbox: enable fwu banks and mdata regions configs: move to new flash layout and boot flow fwu: DeveloperBox: add support for FWU fwu: provide default fwu_plat_get_bootidx
Masami Hiramatsu (2): FWU: Add FWU metadata access driver for MTD storage regions 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 | 37 ++ configs/synquacer_developerbox_defconfig | 12 +- doc/board/socionext/developerbox.rst | 154 +++++++- doc/mkfwumdata.1 | 89 +++++ drivers/fwu-mdata/Kconfig | 15 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/raw_mtd.c | 269 ++++++++++++++ include/configs/synquacer.h | 10 + include/fwu.h | 32 ++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu.c | 18 + lib/fwu_updates/fwu_mtd.c | 185 ++++++++++ tools/Kconfig | 9 + tools/Makefile | 4 + tools/mkfwumdata.c | 334 ++++++++++++++++++ 18 files changed, 1217 insertions(+), 11 deletions(-) create mode 100644 board/socionext/developerbox/fwu_plat.c create mode 100644 doc/mkfwumdata.1 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 know that Tom has applied this already but I still think there is a work to do here. Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and just use information from mdata. DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Thanks, Michal

Hi Michal,
On Mon, 19 Jun 2023 at 10:02, Michal Simek michal.simek@amd.com wrote:
Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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...
.....
Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
I had, and have, strong doubts about the practicality of 2 images/bank. There aren't enough specification details to explain how only 1-out-of-N images could be updated. And if we always update all images in a bank together, we might as well have them as one composite image. I never got satisfactory clarification from designers and implementers. So, sorry, I can't defend that scenario with my limited knowledge.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and just use information from mdata. DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we use any bit, we'll be in violation of the spec.
However, we can do CRC32 calculations by varying NUM_IMAGES_PER_BANK and NUM_BANKS and find the value pair for which the crc32 field matches! For limiting check-loops and finding corrupted metadata, the .config can carry upper limits on both the options, say MAX_NUM_IMAGES_PER_BANK=5 and MAX_NUM_BANKS=10 for the most special scenario. If we find the approach acceptable, I can cook a patch as proof-of-concept.
cheers.

Sorry for being late to the party,
+cc Jose who maintains DEN0118
On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
Hi Michal,
On Mon, 19 Jun 2023 at 10:02, Michal Simek michal.simek@amd.com wrote:
Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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...
.....
Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
I had, and have, strong doubts about the practicality of 2 images/bank. There aren't enough specification details to explain how only 1-out-of-N images could be updated. And if we always update all images in a bank together, we might as well have them as one composite image. I never got satisfactory clarification from designers and implementers. So, sorry, I can't defend that scenario with my limited knowledge.
This is intentionally left out, as we consider it a platform policy. For example you could update a single bank and copy over the remaining firmware images from a different ban. There's nothing on the spec that prohibits you from doing so, but I personally think it's a bad idea. Keep in mind there's a document hosted here[0] which explains the update flow and documents what we have as code in U-Boot.
As far as individual firmware components go now, do you have any useful usecase? The general guidance of [0] is construct a firmware image, only update the components you want while leaving the rest on the same revisions and update the entire firmware. The reason is that firmware
Updating a single image of another bank is not as easy as it sounds. Firmware components nowadays, whether we like it or not, depend on each other. Since firmware updates are not so often and fast, we didn't see any gains from over complicating the spec and explicitly describe that case, while dealing with firmware component compatibility at the same time.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and just use information from mdata. DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we use any bit, we'll be in violation of the spec.
Yes this is unfortunate. We did have similar concerns on when drafting the spec, however we never had a solid usecase to justify this. Arm and Linaro and working on a v2 (which we try to make compatible with v1) which addresses this shortcoming. Maybe Jose can chime in.
However, we can do CRC32 calculations by varying NUM_IMAGES_PER_BANK and NUM_BANKS and find the value pair for which the crc32 field matches! For limiting check-loops and finding corrupted metadata, the .config can carry upper limits on both the options, say MAX_NUM_IMAGES_PER_BANK=5 and MAX_NUM_BANKS=10 for the most special scenario. If we find the approach acceptable, I can cook a patch as proof-of-concept.
cheers.
[0] https://gitlab.com/Linaro/trustedsubstrate/mbfw/uploads/3d0d7d11ca9874dc9115...
Thanks /Ilias

On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Sorry for being late to the party,
+cc Jose who maintains DEN0118
On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
Hi Michal,
On Mon, 19 Jun 2023 at 10:02, Michal Simek michal.simek@amd.com wrote:
Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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...
.....
Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
I had, and have, strong doubts about the practicality of 2 images/bank. There aren't enough specification details to explain how only 1-out-of-N images could be updated. And if we always update all images in a bank together, we might as well have them as one composite image. I never got satisfactory clarification from designers and implementers. So, sorry, I can't defend that scenario with my limited knowledge.
This is intentionally left out, as we consider it a platform policy. For example you could update a single bank and copy over the remaining firmware images from a different ban. There's nothing on the spec that prohibits you from doing so, but I personally think it's a bad idea.
Me too.
Keep in mind there's a document hosted here[0] which explains the update flow and documents what we have as code in U-Boot.
As far as individual firmware components go now, do you have any useful usecase?
No. And I don't think current A/B update implementation in u-boot even has helpers for platforms to implement multiple images per bank. Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK config -- which will always be set to 1 I predict ;)
The general guidance of [0] is construct a firmware image, only update the components you want while leaving the rest on the same revisions and update the entire firmware. The reason is that firmware
Updating a single image of another bank is not as easy as it sounds.
exactly.
Firmware components nowadays, whether we like it or not, depend on each other. Since firmware updates are not so often and fast, we didn't see any gains from over complicating the spec and explicitly describe that case, while dealing with firmware component compatibility at the same time.
Totally. As I said, I don't believe in the practicality of more than 1 image/bank. So we are on the same page.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and just use information from mdata. DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we use any bit, we'll be in violation of the spec.
Yes this is unfortunate. We did have similar concerns on when drafting the spec, however we never had a solid usecase to justify this. Arm and Linaro and working on a v2 (which we try to make compatible with v1) which addresses this shortcoming. Maybe Jose can chime in.
OK. If we have, say, reserved[0] as last Image and reserved[1] as last Bank, we could get rid of the two compile-time configs.
Thanks.

On 6/20/23 16:14, Jassi Brar wrote:
On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Sorry for being late to the party,
+cc Jose who maintains DEN0118
On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
Hi Michal,
On Mon, 19 Jun 2023 at 10:02, Michal Simek michal.simek@amd.com wrote:
Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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...
.....
Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
I had, and have, strong doubts about the practicality of 2 images/bank. There aren't enough specification details to explain how only 1-out-of-N images could be updated. And if we always update all images in a bank together, we might as well have them as one composite image. I never got satisfactory clarification from designers and implementers. So, sorry, I can't defend that scenario with my limited knowledge.
This is intentionally left out, as we consider it a platform policy. For example you could update a single bank and copy over the remaining firmware images from a different ban. There's nothing on the spec that prohibits you from doing so, but I personally think it's a bad idea.
Me too.
Keep in mind there's a document hosted here[0] which explains the update flow and documents what we have as code in U-Boot.
As far as individual firmware components go now, do you have any useful usecase?
No. And I don't think current A/B update implementation in u-boot even has helpers for platforms to implement multiple images per bank. Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK config -- which will always be set to 1 I predict ;)
Actually support is there but it requires one thing to patch. I will send that patch when I clean my repo. And I have tested it on our platform.
The general guidance of [0] is construct a firmware image, only update the components you want while leaving the rest on the same revisions and update the entire firmware. The reason is that firmware
Updating a single image of another bank is not as easy as it sounds.
exactly.
Firmware components nowadays, whether we like it or not, depend on each other. Since firmware updates are not so often and fast, we didn't see any gains from over complicating the spec and explicitly describe that case, while dealing with firmware component compatibility at the same time.
Totally. As I said, I don't believe in the practicality of more than 1 image/bank. So we are on the same page.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and just use information from mdata. DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we use any bit, we'll be in violation of the spec.
Yes this is unfortunate. We did have similar concerns on when drafting the spec, however we never had a solid usecase to justify this. Arm and Linaro and working on a v2 (which we try to make compatible with v1) which addresses this shortcoming. Maybe Jose can chime in.
Another way would be to simply decode this information directly from DT fwu-mdata node. Because from that node it is clear how many banks and how many images that particular bank has.
Thanks, Michal

Hi,
-----Original Message----- From: Michal Simek michal.simek@amd.com Sent: Tuesday, June 20, 2023 3:20 PM To: Jassi Brar jaswinder.singh@linaro.org; Ilias Apalodimas ilias.apalodimas@linaro.org Cc: Jose Marinho Jose.Marinho@arm.com; u-boot@lists.denx.de; etienne.carriere@linaro.org; trini@konsulko.com; sjg@chromium.org; sughosh.ganu@linaro.org; xypron.glpk@gmx.de; takahiro.akashi@linaro.org Subject: Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
On 6/20/23 16:14, Jassi Brar wrote:
On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Sorry for being late to the party,
+cc Jose who maintains DEN0118
On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
Hi Michal,
On Mon, 19 Jun 2023 at 10:02, Michal Simek michal.simek@amd.com
wrote:
Hi Jassi,
On 5/31/23 07:28, jaswinder.singh@linaro.org 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
jassisingh
brar@gmail.com/
.....
Firstly I can generate 2 images per one bank which should be pretty much regular capsule update for 2 images. I would expect this should still
work.
And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated this description for DFU
mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw 23a0000 4000000;bank1 raw 2820000 4000000
If you look at size in second entry you will see that it is 8000 instead of 80000 because it is the same image. That's why curious if you have tested any scenario like this.
I had, and have, strong doubts about the practicality of 2 images/bank. There aren't enough specification details to explain how only 1-out-of-N images could be updated. And if we always update all images in a bank together, we might as well have them as one composite image. I never got satisfactory clarification from designers and implementers. So, sorry, I can't defend that scenario with my limited knowledge.
This is intentionally left out, as we consider it a platform policy. For example you could update a single bank and copy over the remaining firmware images from a different ban. There's nothing on the spec that prohibits you from doing so, but I personally think it's a bad
idea.
Me too.
Keep in mind there's a document hosted here[0] which explains the update flow and documents what we have as code in U-Boot.
As far as individual firmware components go now, do you have any useful usecase?
No. And I don't think current A/B update implementation in u-boot even has helpers for platforms to implement multiple images per bank. Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK config -- which will always be set to 1 I predict ;)
Actually support is there but it requires one thing to patch. I will send that patch when I clean my repo. And I have tested it on our platform.
The general guidance of [0] is construct a firmware image, only update the components you want while leaving the rest on the same revisions and update the entire firmware. The reason is that firmware
Updating a single image of another bank is not as easy as it sounds.
exactly.
Firmware components nowadays, whether we like it or not, depend on
each other.
Since firmware updates are not so often and fast, we didn't see any gains from over complicating the spec and explicitly describe that case, while dealing with firmware component compatibility at the same
time.
Totally. As I said, I don't believe in the practicality of more than 1 image/bank. So we are on the same page.
Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS and
CONFIG_FWU_NUM_IMAGES_PER_BANK because it
pretty much enforcing number of banks and images for every platform and prevent creating one u-boot which works on different boards and
just use information from mdata.
DEN0118 doesn't show any field with this information but do you think that would be possible to extract that information from there based on for example reserved or accepted field?
Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we use any bit, we'll be in violation of the spec.
Yes this is unfortunate. We did have similar concerns on when drafting the spec, however we never had a solid usecase to justify this. Arm and Linaro and working on a v2 (which we try to make compatible with v1) which addresses this shortcoming. Maybe Jose can
chime in.
Another way would be to simply decode this information directly from DT fwu-mdata node. Because from that node it is clear how many banks and how many images that particular bank has.
The FW update document (DEN0118) is in the process of being released. It defines the v2 of the metadata data-structure. Version 2 should better fit the deployment models since it has a field for 'number of banks' and another for 'number of images per bank'. Happy to discuss in general, and in particular if we find any gaps 😊!
Regards, Jose
Thanks, Michal
participants (6)
-
Ilias Apalodimas
-
Jassi Brar
-
jaswinder.singh@linaro.org
-
Jose Marinho
-
Michal Simek
-
Tom Rini