From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 500CAC433F5 for ; Tue, 8 Feb 2022 12:07:46 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 725AF83BB9; Tue, 8 Feb 2022 13:07:43 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="UrwU6VY8"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0979C83C85; Tue, 8 Feb 2022 13:07:42 +0100 (CET) Received: from mail-io1-xd2a.google.com (mail-io1-xd2a.google.com [IPv6:2607:f8b0:4864:20::d2a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BAD5783917 for ; Tue, 8 Feb 2022 13:07:34 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sughosh.ganu@linaro.org Received: by mail-io1-xd2a.google.com with SMTP id h7so20794736iof.3 for ; Tue, 08 Feb 2022 04:07:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=AIr60vbuPsVDVrYzA/7uTWX5lfGl2AWRGaUy1bbElBc=; b=UrwU6VY8GYZ9Hp2g6A2OLCZH7qGyCki2TRBjougu2lxo8/qvRgLzvg1knzv+bnjuLQ fGBvN3R1IJimo6qZsWZXXstQmLLcJIUTKXRbaIccVL9ffYo8QmMaLr6L9okmNPPEGvT5 uKLhEhMqQ3HM5CQlusWreZPkUsr0TB1r+5kX7+NArgKXMHovqVEh0Ni0GviOkYYKQpXU WTGigo/MdWdIvg0LcNm4sElJhlYec7NdtjvSAn1f6AG68KJ+QbGGI0c29QzKXC9dH5Ol 6zvzYOYQpaQskT2xhe0m2zQA1zXl9NdCifv1lqltWBesPzq9dulhKTUEG7hr+3JA+QHf 6/Gw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=AIr60vbuPsVDVrYzA/7uTWX5lfGl2AWRGaUy1bbElBc=; b=Wg8rl8VuVIoBVHpwU4XdC9OnjDxktIADOgqKJ8y70eya4MIaxL3lbYXREaPYyOSDyA t+ln8KgNiNWgWhgyEKBDiIz1KO2qFkzUAWYj04uFhjDyDL2mi1FbetGLDo+B0V4hxAUi DwAxfJfJ79mbcbPMQivqeOqV8TGvCVAm8MTQYCmtDmd677WOQvECa5xSvOX9cH8Zt4am qd2dd/ZUIwjVNHfthV+zirUf+Ev8aijsZA0ZWu+iTsPGgQ9ScAmgrhk2qrato8Y65kWY nmDLuMvdH+QMuRgrmXt2Ty3SwZ9Vzd66C+eR4Ik1XCEgZx8izWdlbEuCYmsemKkHNKnQ Jdxw== X-Gm-Message-State: AOAM531qY7V8IoOKY82wJHSEL9T+39FQqzw6cl9esLnzfj9StIEzFXn8 6NGA+jcdqOdOhNfVME7JybjzQVoLzyY6a54w3e0aWw== X-Google-Smtp-Source: ABdhPJxqHS99J28DYV4r0UXHMy6bogO2bQUBn5305m1tJDG0jEe/cCsl7t76wkoOREN2ZJsJyFupGDvUBdios0vpr6E= X-Received: by 2002:a05:6638:408a:: with SMTP id m10mr2107978jam.16.1644322053208; Tue, 08 Feb 2022 04:07:33 -0800 (PST) MIME-Version: 1.0 References: <20220207182001.31270-1-sughosh.ganu@linaro.org> <20220207182001.31270-2-sughosh.ganu@linaro.org> <654bf1ed-2ea3-f37f-c74b-01b5ba10dfda@xilinx.com> <5131bfe1-95e0-0954-51a6-8140546ca5f0@xilinx.com> In-Reply-To: <5131bfe1-95e0-0954-51a6-8140546ca5f0@xilinx.com> From: Sughosh Ganu Date: Tue, 8 Feb 2022 17:37:21 +0530 Message-ID: Subject: Re: [PATCH v4 01/11] FWU: Add FWU metadata structure and driver for accessing metadata To: Michal Simek Cc: U-Boot , Heinrich Schuchardt , Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , AKASHI Takahiro , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Tue, 8 Feb 2022 at 17:29, Michal Simek wrote: > > > > On 2/8/22 12:54, Sughosh Ganu wrote: > > On Tue, 8 Feb 2022 at 17:15, Michal Simek wro= te: > >> > >> > >> > >> On 2/8/22 12:38, Sughosh Ganu wrote: > >>> On Tue, 8 Feb 2022 at 17:01, Michal Simek wrote: > >>>> > >>>> po 7. 2. 2022 v 19:21 odes=C3=ADlatel Sughosh Ganu napsal: > >>>>> > >>>>> In the FWU Multi Bank Update feature, the information about the > >>>>> updatable images is stored as part of the metadata, which is stored= on > >>>>> a dedicated partition. Add the metadata structure, and a driver mod= el > >>>>> uclass which provides functions to access the metadata. These are > >>>>> generic API's, and implementations can be added based on parameters > >>>>> like how the metadata partition is accessed and what type of storag= e > >>>>> device houses the metadata. > >>>>> > >>>>> A device tree node fwu-mdata has been added, which is used for > >>>>> pointing to the storage device which contains the FWU metadata. The > >>>>> fwu-mdata node is u-boot specific, and can be added the platform's > >>>>> u-boot dtsi file. > >>>>> > >>>>> Signed-off-by: Sughosh Ganu > >>>>> --- > >>>>> > >>>>> Changes since V3: > >>>>> * Move the FWU metadata access to driver model > >>>>> * Get the storage device containing the metadata from a device tree > >>>>> property instead of a platform helper function > >>>>> > >>>>> arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi | 7 + > >>>>> .../firmware/fwu-mdata.txt | 18 + > >>>>> drivers/Kconfig | 2 + > >>>>> drivers/Makefile | 1 + > >>>>> drivers/fwu-mdata/Kconfig | 7 + > >>>>> drivers/fwu-mdata/Makefile | 6 + > >>>>> drivers/fwu-mdata/fwu-mdata-uclass.c | 434 ++++++++++++= ++++++ > >>>>> include/dm/uclass-id.h | 1 + > >>>>> include/fwu.h | 51 ++ > >>>>> include/fwu_mdata.h | 67 +++ > >>>>> 10 files changed, 594 insertions(+) > >>>>> create mode 100644 doc/device-tree-bindings/firmware/fwu-mdata.t= xt > >>>>> create mode 100644 drivers/fwu-mdata/Kconfig > >>>>> create mode 100644 drivers/fwu-mdata/Makefile > >>>>> create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c > >>>>> create mode 100644 include/fwu.h > >>>>> create mode 100644 include/fwu_mdata.h > >>>>> > >>>>> diff --git a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi b/arch/arm/dt= s/stm32mp157c-dk2-u-boot.dtsi > >>>>> index 06ef3a4095..3bec6107f7 100644 > >>>>> --- a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi > >>>>> +++ b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi > >>>>> @@ -4,3 +4,10 @@ > >>>>> */ > >>>>> > >>>>> #include "stm32mp157a-dk1-u-boot.dtsi" > >>>>> + > >>>>> +/ { > >>>>> + fwu-mdata { > >>>>> + compatible =3D "u-boot,fwu-mdata"; > >>>>> + fwu-mdata-store =3D <&sdmmc1>; > >>>>> + }; > >>>>> +}; > >>>>> diff --git a/doc/device-tree-bindings/firmware/fwu-mdata.txt b/doc/= device-tree-bindings/firmware/fwu-mdata.txt > >>>>> new file mode 100644 > >>>>> index 0000000000..c766b595ef > >>>>> --- /dev/null > >>>>> +++ b/doc/device-tree-bindings/firmware/fwu-mdata.txt > >>>>> @@ -0,0 +1,18 @@ > >>>>> +FWU Metadata Access Devicetree Binding > >>>>> + > >>>>> +The FWU Multi Bank Update feature uses a metadata structure, store= d on > >>>>> +a separate partition for keeping information on the set of updatab= le > >>>>> +images. The device tree node provides information on the storage > >>>>> +device that contains the FWU metadata. > >>>>> + > >>>>> +Required properties : > >>>>> + > >>>>> +- compatible : "u-boot,fwu-mdata"; > >>>>> +- fwu-mdata-store : should point to the storage device which conta= ins > >>>>> + the FWU metadata partition. > >>>>> + > >>>>> +Example : > >>>>> + fwu-mdata { > >>>>> + compatible =3D "u-boot,fwu-mdata"; > >>>>> + fwu-mdata-store =3D <&sdmmc1>; > >>>>> + }; > >>>>> diff --git a/drivers/Kconfig b/drivers/Kconfig > >>>>> index b26ca8cf70..adc6079ecf 100644 > >>>>> --- a/drivers/Kconfig > >>>>> +++ b/drivers/Kconfig > >>>>> @@ -42,6 +42,8 @@ source "drivers/firmware/Kconfig" > >>>>> > >>>>> source "drivers/fpga/Kconfig" > >>>>> > >>>>> +source "drivers/fwu-mdata/Kconfig" > >>>>> + > >>>>> source "drivers/gpio/Kconfig" > >>>>> > >>>>> source "drivers/hwspinlock/Kconfig" > >>>>> diff --git a/drivers/Makefile b/drivers/Makefile > >>>>> index 4e7cf28440..56f0f04874 100644 > >>>>> --- a/drivers/Makefile > >>>>> +++ b/drivers/Makefile > >>>>> @@ -81,6 +81,7 @@ obj-y +=3D cache/ > >>>>> obj-$(CONFIG_CPU) +=3D cpu/ > >>>>> obj-y +=3D crypto/ > >>>>> obj-$(CONFIG_FASTBOOT) +=3D fastboot/ > >>>>> +obj-$(CONFIG_DM_FWU_MDATA) +=3D fwu-mdata/ > >>>>> obj-y +=3D misc/ > >>>>> obj-$(CONFIG_MMC) +=3D mmc/ > >>>>> obj-$(CONFIG_NVME) +=3D nvme/ > >>>>> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig > >>>>> new file mode 100644 > >>>>> index 0000000000..d6a21c8e19 > >>>>> --- /dev/null > >>>>> +++ b/drivers/fwu-mdata/Kconfig > >>>>> @@ -0,0 +1,7 @@ > >>>>> +config DM_FWU_MDATA > >>>>> + bool "Driver support for accessing FWU Metadata" > >>>>> + depends on DM > >>>>> + help > >>>>> + Enable support for accessing FWU Metadata partitions. The > >>>>> + FWU Metadata partitions reside on the same storage device > >>>>> + which contains the other FWU updatable firmware images. > >>>>> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefil= e > >>>>> new file mode 100644 > >>>>> index 0000000000..7fec7171f4 > >>>>> --- /dev/null > >>>>> +++ b/drivers/fwu-mdata/Makefile > >>>>> @@ -0,0 +1,6 @@ > >>>>> +# SPDX-License-Identifier: GPL-2.0+ > >>>>> +# > >>>>> +# Copyright (c) 2022, Linaro Limited > >>>>> +# > >>>>> + > >>>>> +obj-$(CONFIG_DM_FWU_MDATA) +=3D fwu-mdata-uclass.o > >>>>> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mda= ta/fwu-mdata-uclass.c > >>>>> new file mode 100644 > >>>>> index 0000000000..64b3051ecf > >>>>> --- /dev/null > >>>>> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > >>>>> @@ -0,0 +1,434 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0+ > >>>>> +/* > >>>>> + * Copyright (c) 2022, Linaro Limited > >>>>> + */ > >>>>> + > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> + > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> + > >>>>> +#define IMAGE_ACCEPT_SET BIT(0) > >>>>> +#define IMAGE_ACCEPT_CLEAR BIT(1) > >>>>> + > >>>>> +static int fwu_get_dev_ops(struct udevice **dev, > >>>>> + const struct fwu_mdata_ops **ops) > >>>>> +{ > >>>>> + int ret; > >>>>> + > >>>>> + ret =3D uclass_get_device(UCLASS_FWU_MDATA, 0, dev); > >>>>> + if (ret) { > >>>>> + log_debug("Cannot find fwu device\n"); > >>>>> + return ret; > >>>>> + } > >>>>> + > >>>>> + if ((*ops =3D device_get_ops(*dev)) =3D=3D NULL) { > >>>>> + log_debug("Cannot get fwu device ops\n"); > >>>>> + return -ENOSYS; > >>>>> + } > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_verify_mdata() - Verify the FWU metadata > >>>>> + * @mdata: FWU metadata structure > >>>>> + * @pri_part: FWU metadata partition is primary or secondary > >>>>> + * > >>>>> + * Verify the FWU metadata by computing the CRC32 for the metadata > >>>>> + * structure and comparing it against the CRC32 value stored as pa= rt > >>>>> + * of the structure. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) > >>>>> +{ > >>>>> + u32 calc_crc32; > >>>>> + void *buf; > >>>>> + > >>>>> + buf =3D &mdata->version; > >>>>> + calc_crc32 =3D crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > >>>>> + > >>>>> + if (calc_crc32 !=3D mdata->crc32) { > >>>>> + log_err("crc32 check failed for %s FWU metadata par= tition\n", > >>>>> + pri_part ? "primary" : "secondary"); > >>>>> + return -1; > >>>>> + } > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_get_active_index() - Get active_index from the FWU metadata > >>>>> + * @active_idx: active_index value to be read > >>>>> + * > >>>>> + * Read the active_index field from the FWU metadata and place it = in > >>>>> + * the variable pointed to be the function argument. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_get_active_index(u32 *active_idx) > >>>>> +{ > >>>>> + int ret; > >>>>> + struct fwu_mdata *mdata =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_mdata(&mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Unable to get valid FWU metadata\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Found the FWU metadata partition, now read the active_in= dex > >>>>> + * value > >>>>> + */ > >>>>> + *active_idx =3D mdata->active_index; > >>>>> + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > >>>>> + log_err("Active index value read is incorrect\n"); > >>>>> + ret =3D -EINVAL; > >>>>> + } > >>>>> + > >>>>> +out: > >>>>> + free(mdata); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_update_active_index() - Update active_index from the FWU me= tadata > >>>>> + * @active_idx: active_index value to be updated > >>>>> + * > >>>>> + * Update the active_index field in the FWU metadata > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_update_active_index(u32 active_idx) > >>>>> +{ > >>>>> + int ret; > >>>>> + void *buf; > >>>>> + struct fwu_mdata *mdata =3D NULL; > >>>>> + > >>>>> + if (active_idx > CONFIG_FWU_NUM_BANKS - 1) { > >>>>> + log_err("Active index value to be updated is incorr= ect\n"); > >>>>> + return -1; > >>>>> + } > >>>>> + > >>>>> + ret =3D fwu_get_mdata(&mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Unable to get valid FWU metadata\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Update the active index and previous_active_index fields > >>>>> + * in the FWU metadata > >>>>> + */ > >>>>> + mdata->previous_active_index =3D mdata->active_index; > >>>>> + mdata->active_index =3D active_idx; > >>>>> + > >>>>> + /* > >>>>> + * Calculate the crc32 for the updated FWU metadata > >>>>> + * and put the updated value in the FWU metadata crc32 > >>>>> + * field > >>>>> + */ > >>>>> + buf =3D &mdata->version; > >>>>> + mdata->crc32 =3D crc32(0, buf, sizeof(*mdata) - sizeof(u32)= ); > >>>>> + > >>>>> + /* > >>>>> + * Now write this updated FWU metadata to both the > >>>>> + * FWU metadata partitions > >>>>> + */ > >>>>> + ret =3D fwu_update_mdata(mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Failed to update FWU metadata partitions\n= "); > >>>>> + ret =3D -EIO; > >>>>> + } > >>>>> + > >>>>> +out: > >>>>> + free(mdata); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_get_image_alt_num() - Get the dfu alt number to be used for= capsule update > >>>>> + * @image_type_id: image guid as passed in the capsule > >>>>> + * @update_bank: Bank to which the update is to be made > >>>>> + * @alt_num: The alt_num for the image > >>>>> + * > >>>>> + * Based on the guid value passed in the capsule, along with the b= ank to which the > >>>>> + * image needs to be updated, get the dfu alt number which will be= used for the > >>>>> + * capsule update > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_ban= k, > >>>>> + int *alt_num) > >>>>> +{ > >>>>> + int ret; > >>>>> + const struct fwu_mdata_ops *ops =3D NULL; > >>>>> + struct udevice *dev =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_dev_ops(&dev, &ops); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (!ops->get_image_alt_num) { > >>>>> + log_err("get_image_alt_num() method not defined\n")= ; > >>>>> + return -ENOSYS; > >>>>> + } > >>>>> + > >>>>> + return ops->get_image_alt_num(dev, image_type_id, > >>>>> + update_bank, alt_num); > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_mdata_check() - Check if the FWU metadata is valid > >>>>> + * > >>>>> + * Validate both copies of the FWU metadata. If one of the copies > >>>>> + * has gone bad, restore it from the other bad copy. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_mdata_check(void) > >>>>> +{ > >>>>> + int ret; > >>>>> + struct udevice *dev =3D NULL; > >>>>> + const struct fwu_mdata_ops *ops =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_dev_ops(&dev, &ops); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (!ops->mdata_check) { > >>>>> + log_err("mdata_check() method not defined\n"); > >>>>> + return -ENOSYS; > >>>>> + } > >>>>> + > >>>>> + return ops->mdata_check(dev); > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_revert_boot_index() - Revert the active index in the FWU me= tadata > >>>>> + * > >>>>> + * Revert the active_index value in the FWU metadata, by swapping = the values > >>>>> + * of active_index and previous_active_index in both copies of the > >>>>> + * FWU metadata. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_revert_boot_index(void) > >>>>> +{ > >>>>> + int ret; > >>>>> + void *buf; > >>>>> + u32 cur_active_index; > >>>>> + struct fwu_mdata *mdata =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_mdata(&mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Unable to get valid FWU metadata\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Swap the active index and previous_active_index fields > >>>>> + * in the FWU metadata > >>>>> + */ > >>>>> + cur_active_index =3D mdata->active_index; > >>>>> + mdata->active_index =3D mdata->previous_active_index; > >>>>> + mdata->previous_active_index =3D cur_active_index; > >>>>> + > >>>>> + /* > >>>>> + * Calculate the crc32 for the updated FWU metadata > >>>>> + * and put the updated value in the FWU metadata crc32 > >>>>> + * field > >>>>> + */ > >>>>> + buf =3D &mdata->version; > >>>>> + mdata->crc32 =3D crc32(0, buf, sizeof(*mdata) - sizeof(u32)= ); > >>>>> + > >>>>> + /* > >>>>> + * Now write this updated FWU metadata to both the > >>>>> + * FWU metadata partitions > >>>>> + */ > >>>>> + ret =3D fwu_update_mdata(mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Failed to update FWU metadata partitions\n= "); > >>>>> + ret =3D -EIO; > >>>>> + } > >>>>> + > >>>>> +out: > >>>>> + free(mdata); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_set_clear_image_accept() - Set or Clear the Acceptance bit = for the image > >>>>> + * @img_type_id: Guid of the image type for which the accepted bit= is to be > >>>>> + * set or cleared > >>>>> + * @bank: Bank of which the image's Accept bit is to be set or cle= ared > >>>>> + * @action: Action which specifies whether image's Accept bit is t= o be set or > >>>>> + * cleared > >>>>> + * > >>>>> + * Set/Clear the accepted bit for the image specified by the img_g= uid parameter. > >>>>> + * This indicates acceptance or rejection of image for subsequent = boots by some > >>>>> + * governing component like OS(or firmware). > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +static int fwu_set_clear_image_accept(efi_guid_t *img_type_id, > >>>>> + u32 bank, u8 action) > >>>>> +{ > >>>>> + void *buf; > >>>>> + int ret, i; > >>>>> + u32 nimages; > >>>>> + struct fwu_mdata *mdata =3D NULL; > >>>>> + struct fwu_image_entry *img_entry; > >>>>> + struct fwu_image_bank_info *img_bank_info; > >>>>> + > >>>>> + ret =3D fwu_get_mdata(&mdata); > >>>>> + if (ret < 0) { > >>>>> + log_err("Unable to get valid FWU metadata\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + > >>>>> + nimages =3D CONFIG_FWU_NUM_IMAGES_PER_BANK; > >>>>> + img_entry =3D &mdata->img_entry[0]; > >>>>> + for (i =3D 0; i < nimages; i++) { > >>>>> + if (!guidcmp(&img_entry[i].image_type_uuid, img_typ= e_id)) { > >>>>> + img_bank_info =3D &img_entry[i].img_bank_in= fo[bank]; > >>>>> + if (action =3D=3D IMAGE_ACCEPT_SET) > >>>>> + img_bank_info->accepted |=3D FWU_IM= AGE_ACCEPTED; > >>>>> + else > >>>>> + img_bank_info->accepted =3D 0; > >>>>> + > >>>>> + buf =3D &mdata->version; > >>>>> + mdata->crc32 =3D crc32(0, buf, sizeof(*mdat= a) - > >>>>> + sizeof(u32)); > >>>>> + > >>>>> + ret =3D fwu_update_mdata(mdata); > >>>>> + goto out; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> + /* Image not found */ > >>>>> + ret =3D -EINVAL; > >>>>> + > >>>>> +out: > >>>>> + free(mdata); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_accept_image() - Set the Acceptance bit for the image > >>>>> + * @img_type_id: Guid of the image type for which the accepted bit= is to be > >>>>> + * cleared > >>>>> + * @bank: Bank of which the image's Accept bit is to be set > >>>>> + * > >>>>> + * Set the accepted bit for the image specified by the img_guid pa= rameter. This > >>>>> + * indicates acceptance of image for subsequent boots by some gove= rning component > >>>>> + * like OS(or firmware). > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank) > >>>>> +{ > >>>>> + return fwu_set_clear_image_accept(img_type_id, bank, > >>>>> + IMAGE_ACCEPT_SET); > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_clear_accept_image() - Clear the Acceptance bit for the ima= ge > >>>>> + * @img_type_id: Guid of the image type for which the accepted bit= is to be > >>>>> + * cleared > >>>>> + * @bank: Bank of which the image's Accept bit is to be cleared > >>>>> + * > >>>>> + * Clear the accepted bit for the image type specified by the img_= type_id parameter. > >>>>> + * This function is called after the image has been updated. The a= ccepted bit is > >>>>> + * cleared to be set subsequently after passing the image acceptan= ce criteria, by > >>>>> + * either the OS(or firmware) > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > >>>>> +{ > >>>>> + return fwu_set_clear_image_accept(img_type_id, bank, > >>>>> + IMAGE_ACCEPT_CLEAR); > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_get_mdata() - Get a FWU metadata copy > >>>>> + * @mdata: Copy of the FWU metadata > >>>>> + * > >>>>> + * Get a valid copy of the FWU metadata. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_get_mdata(struct fwu_mdata **mdata) > >>>>> +{ > >>>>> + int ret; > >>>>> + struct udevice *dev =3D NULL; > >>>>> + const struct fwu_mdata_ops *ops =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_dev_ops(&dev, &ops); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (!ops->get_mdata) { > >>>>> + log_err("get_mdata() method not defined\n"); > >>>>> + return -ENOSYS; > >>>>> + } > >>>>> + > >>>>> + return ops->get_mdata(dev, mdata); > >>>>> +} > >>>>> + > >>>>> +/** > >>>>> + * fwu_update_mdata() - Update the FWU metadata > >>>>> + * @mdata: Copy of the FWU metadata > >>>>> + * > >>>>> + * Update the FWU metadata structure by writing to the > >>>>> + * FWU metadata partitions. > >>>>> + * > >>>>> + * Return: 0 if OK, -ve on error > >>>>> + * > >>>>> + */ > >>>>> +int fwu_update_mdata(struct fwu_mdata *mdata) > >>>>> +{ > >>>>> + int ret; > >>>>> + struct udevice *dev =3D NULL; > >>>>> + const struct fwu_mdata_ops *ops =3D NULL; > >>>>> + > >>>>> + ret =3D fwu_get_dev_ops(&dev, &ops); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (!ops->update_mdata) { > >>>>> + log_err("get_mdata() method not defined\n"); > >>>>> + return -ENOSYS; > >>>>> + } > >>>>> + > >>>>> + return ops->update_mdata(dev, mdata); > >>>>> +} > >>>>> + > >>>>> +UCLASS_DRIVER(fwu_mdata) =3D { > >>>>> + .id =3D UCLASS_FWU_MDATA, > >>>>> + .name =3D "fwu-mdata", > >>>>> +}; > >>>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > >>>>> index 0e26e1d138..d0ab1c9235 100644 > >>>>> --- a/include/dm/uclass-id.h > >>>>> +++ b/include/dm/uclass-id.h > >>>>> @@ -54,6 +54,7 @@ enum uclass_id { > >>>>> UCLASS_ETH_PHY, /* Ethernet PHY device */ > >>>>> UCLASS_FIRMWARE, /* Firmware */ > >>>>> UCLASS_FS_FIRMWARE_LOADER, /* Generic loader= */ > >>>>> + UCLASS_FWU_MDATA, /* FWU Metadata Access */ > >>>>> UCLASS_GPIO, /* Bank of general-purpose I/O pi= ns */ > >>>>> UCLASS_HASH, /* Hash device */ > >>>>> UCLASS_HWSPINLOCK, /* Hardware semaphores */ > >>>>> diff --git a/include/fwu.h b/include/fwu.h > >>>>> new file mode 100644 > >>>>> index 0000000000..5a99c579fc > >>>>> --- /dev/null > >>>>> +++ b/include/fwu.h > >>>>> @@ -0,0 +1,51 @@ > >>>>> +/* SPDX-License-Identifier: GPL-2.0+ */ > >>>>> +/* > >>>>> + * Copyright (c) 2022, Linaro Limited > >>>>> + */ > >>>>> + > >>>>> +#if !defined _FWU_H_ > >>>>> +#define _FWU_H_ > >>>>> + > >>>>> +#include > >>>>> +#include > >>>>> + > >>>>> +#include > >>>>> + > >>>>> +struct fwu_mdata; > >>>>> +struct udevice; > >>>>> + > >>>>> +/** > >>>>> + * @get_image_alt_num: get the alt number to be used for the image > >>>>> + * @mdata_check: check the validity of the FWU metadata partitions > >>>>> + * @get_mdata() - Get a FWU metadata copy > >>>>> + * @update_mdata() - Update the FWU metadata copy > >>>>> + */ > >>>>> +struct fwu_mdata_ops { > >>>>> + int (*get_image_alt_num)(struct udevice *dev, efi_guid_t im= age_type_id, > >>>>> + u32 update_bank, int *alt_num); > >>>>> + > >>>>> + int (*mdata_check)(struct udevice *dev); > >>>>> + > >>>>> + int (*get_mdata)(struct udevice *dev, struct fwu_mdata **md= ata); > >>>>> + > >>>>> + int (*update_mdata)(struct udevice *dev, struct fwu_mdata *= mdata); > >>>>> +}; > >>>>> + > >>>>> +#define FWU_MDATA_VERSION 0x1 > >>>>> + > >>>>> +#define FWU_MDATA_GUID \ > >>>>> + EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > >>>>> + 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > >>>>> + > >>>>> +int fwu_get_mdata(struct fwu_mdata **mdata); > >>>>> +int fwu_update_mdata(struct fwu_mdata *mdata); > >>>>> +int fwu_get_active_index(u32 *active_idx); > >>>>> +int fwu_update_active_index(u32 active_idx); > >>>>> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_ban= k, > >>>>> + int *alt_num); > >>>>> +int fwu_mdata_check(void); > >>>>> +int fwu_revert_boot_index(void); > >>>>> +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); > >>>>> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > >>>>> + > >>>>> +#endif /* _FWU_H_ */ > >>>>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > >>>>> new file mode 100644 > >>>>> index 0000000000..701efbba03 > >>>>> --- /dev/null > >>>>> +++ b/include/fwu_mdata.h > >>>>> @@ -0,0 +1,67 @@ > >>>>> +/* SPDX-License-Identifier: GPL-2.0+ */ > >>>>> +/* > >>>>> + * Copyright (c) 2022, Linaro Limited > >>>>> + */ > >>>>> + > >>>>> +#if !defined _FWU_MDATA_H_ > >>>>> +#define _FWU_MDATA_H_ > >>>>> + > >>>>> +#include > >>>>> + > >>>>> +/** > >>>>> + * struct fwu_image_bank_info - firmware image information > >>>>> + * @image_uuid: Guid value of the image in this bank > >>>>> + * @accepted: Acceptance status of the image > >>>>> + * @reserved: Reserved > >>>>> + * > >>>>> + * The structure contains image specific fields which are > >>>>> + * used to identify the image and to specify the image's > >>>>> + * acceptance status > >>>>> + */ > >>>>> +struct fwu_image_bank_info { > >>>>> + efi_guid_t image_uuid; > >>>>> + uint32_t accepted; > >>>>> + uint32_t reserved; > >>>>> +} __attribute__((__packed__)); > >>>>> + > >>>>> +/** > >>>>> + * struct fwu_image_entry - information for a particular type of i= mage > >>>>> + * @image_type_uuid: Guid value for identifying the image type > >>>>> + * @location_uuid: Guid of the storage volume where the image is l= ocated > >>>>> + * @img_bank_info: Array containing properties of images > >>>>> + * > >>>>> + * This structure contains information on various types of updatab= le > >>>>> + * firmware images. Each image type then contains an array of imag= e > >>>>> + * information per bank. > >>>>> + */ > >>>>> +struct fwu_image_entry { > >>>>> + efi_guid_t image_type_uuid; > >>>>> + efi_guid_t location_uuid; > >>>>> + struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BAN= KS]; > >>>>> +} __attribute__((__packed__)); > >>>>> + > >>>>> +/** > >>>>> + * struct fwu_mdata - FWU metadata structure for multi-bank update= s > >>>>> + * @crc32: crc32 value for the FWU metadata > >>>>> + * @version: FWU metadata version > >>>>> + * @active_index: Index of the bank currently used for booting ima= ges > >>>>> + * @previous_active_inde: Index of the bank used before the curren= t bank > >>>>> + * being used for booting > >>>>> + * @img_entry: Array of information on various firmware images tha= t can > >>>>> + * be updated > >>>>> + * > >>>>> + * This structure is used to store all the needed information for = performing > >>>>> + * multi bank updates on the platform. This contains info on the b= ank being > >>>>> + * used to boot along with the information needed for identificati= on of > >>>>> + * individual images > >>>>> + */ > >>>>> +struct fwu_mdata { > >>>>> + uint32_t crc32; > >>>>> + uint32_t version; > >>>>> + uint32_t active_index; > >>>>> + uint32_t previous_active_index; > >>>>> + > >>>>> + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_= BANK]; > >>>>> +} __attribute__((__packed__)); > >>>>> + > >>>>> +#endif /* _FWU_MDATA_H_ */ > >>>>> -- > >>>>> 2.17.1 > >>>>> > >>>> > >>>> One more thing. run kernel-doc to validate your description. > >>>> [u-boot](eeee)$ ./scripts/kernel-doc -v -man > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c 1>/dev/null > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:41: info: Scanning doc for fwu_= verify_mdata > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:70: info: Scanning doc for > >>>> fwu_get_active_index > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:107: info: Scanning doc for > >>>> fwu_update_active_index > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:164: info: Scanning doc for > >>>> fwu_get_image_alt_num > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:197: info: Scanning doc for fwu= _mdata_check > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:202: warning: contents before s= ections > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:224: info: Scanning doc for > >>>> fwu_revert_boot_index > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:230: warning: contents before s= ections > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:279: info: Scanning doc for > >>>> fwu_set_clear_image_accept > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:338: info: Scanning doc for > >>>> fwu_accept_image > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:357: info: Scanning doc for > >>>> fwu_clear_accept_image > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:377: info: Scanning doc for fwu= _get_mdata > >>>> drivers/fwu-mdata/fwu-mdata-uclass.c:404: info: Scanning doc for > >>>> fwu_update_mdata > >>>> 2 warnings > >>>> > >>>> when I run buildman over this series it is visible that it is not > >>>> bisectable at all. > >>>> CONFIG_FWU_NUM_BANKS is defined in this patch but the symbol is adde= d > >>>> much later. > >>>> Please make sure every single patch is bisectable. > >>> > >>> > >>> But how is this driver getting built in the first place -- the driver > >>> is not enabling the config symbol which would result in the code > >>> getting built. The idea is to add support for the driver and the > >>> feature, and enable the functionality in the final patch. I have adde= d > >>> each patch separately and built for the qemu arm64 platform without > >>> any issues. > >> > >> first of all I wasn't able to apply all patches because this series is= not there. > >> https://patchwork.ozlabs.org/project/uboot/list/?series=3D281549 > >> (better would be to use links to lore which are much easier to downloa= d) > > > > You can download the mkeficapsule series from linaro's patchwork[1]. > > you depend on 3 series. Much easier is to create a branch and push it som= ewhere. > > >> > >> That's why 2 patches weren't applied. > >> > >> And I just enabled configs which land in the tree for zynqmp virt and = rebase > >> this patch to be the first one. That's why all the time all existing K= configs > >> were enabled when buildman build them. > > > > Okay my understanding of a patch being bisectable was if the build > > does not break after applying a patch. Please note that on top of my > > series, you will need to define CONFIG_FWU_NUM_BANKS and > > CONFIG_FWU_NUM_IMAGES_PER_BANK. I did not define that on purpose since > > each platform would have it's own specific values. > > Even if this is true you can't use in any patch a macro/value which is no= t > defined and it is not clear what it is. I apply patch 1 and review patch = 1 and > not looking at patch 8 to get what that values are for. My point was that I have not defined these values at all. These will have to be defined on a per platform level. But if you insist, I will define default values, but in my opinion it is better to get a build failure and then set correct values, rather than the build going through with some values which are incorrect for the platform. If you apply the patches as they are, they should not result in any build failures. -sughosh > > Define them as reasonable default for common case or your usecase. Platfo= rm can > set them up when they adopt this approach and they are not happy with you= r defaults. > > Thanks, > Michal > >