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.

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/optio... 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

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

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

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

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;

+}

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/...

-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

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