| Message ID | 20201126184110.30521-7-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: arm64: Add support for uefi capsule update on qemu arm64 platform | expand |
On 11/26/20 7:41 PM, Sughosh Ganu wrote: > The dfu framework uses the dfu_alt_info environment variable to get > information that is needed for performing the firmware update. Set the > dfu_alt_info for the platform to reflect the two mtd partitions > created for the u-boot env and the firmware image. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> I can't see anything QEMU specific in this patch. Why is the code under board/emulation/? Best regards Heinrich > --- > board/emulation/qemu-arm/qemu-arm.c | 55 +++++++++++++++++++++++++++++ > include/configs/qemu-arm.h | 1 + > 2 files changed, 56 insertions(+) > > diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c > index d5ed3eebaf..8cad54c76f 100644 > --- a/board/emulation/qemu-arm/qemu-arm.c > +++ b/board/emulation/qemu-arm/qemu-arm.c > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > +#include <memalign.h> > #include <mtd.h> > > +#define MTDPARTS_LEN 256 > +#define MTDIDS_LEN 128 > + > +#define DFU_ALT_BUF_LEN SZ_1K > + > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > +{ > + struct mtd_info *part; > + bool first = true; > + const char *name; > + int len, partnum = 0; > + > + name = mtd->name; > + len = strlen(buf); > + > + if (buf[0] != '\0') > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > + "mtd %s=", name); > + > + list_for_each_entry(part, &mtd->partitions, node) { > + partnum++; > + if (!first) > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, ";"); > + first = false; > + > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > + "%s part %d", > + part->name, partnum); > + } > +} > + > +void set_dfu_alt_info(char *interface, char *devstr) > +{ > + struct mtd_info *mtd; > + > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > + > + if (env_get("dfu_alt_info")) > + return; > + > + memset(buf, 0, sizeof(buf)); > + > + /* probe all MTD devices */ > + mtd_probe_devices(); > + > + mtd = get_mtd_device_nm("nor0"); > + if (!IS_ERR_OR_NULL(mtd)) > + board_get_alt_info(mtd, buf); > + > + env_set("dfu_alt_info", buf); > + printf("dfu_alt_info set\n"); > +} > + > static void board_get_mtdparts(const char *dev, const char *partition, > char *mtdids, char *mtdparts) > { > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > index 69ff329434..726f985d35 100644 > --- a/include/configs/qemu-arm.h > +++ b/include/configs/qemu-arm.h > @@ -33,6 +33,7 @@ > #include <config_distro_bootcmd.h> > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > +#define CONFIG_SET_DFU_ALT_INFO > #define CONFIG_SYS_MTDPARTS_RUNTIME > #endif > >
On Sat, 5 Dec 2020 at 16:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > The dfu framework uses the dfu_alt_info environment variable to get > > information that is needed for performing the firmware update. Set the > > dfu_alt_info for the platform to reflect the two mtd partitions > > created for the u-boot env and the firmware image. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > I can't see anything QEMU specific in this patch. Why is the code under > board/emulation/? > The device that the board wants to use to set the dfu_alt_info to would be very much board specific. For example, the dfu_alt_info is being set for the qemu platform for an mtd as the interface, with nor0 as the device. Different platforms might have different requirements, like the setting of the variable done for st platforms(board/st/common/stm32mp_dfu.c). I think this information is very much platform specific. -sughosh > > Best regards > > Heinrich > > > --- > > board/emulation/qemu-arm/qemu-arm.c | 55 +++++++++++++++++++++++++++++ > > include/configs/qemu-arm.h | 1 + > > 2 files changed, 56 insertions(+) > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > b/board/emulation/qemu-arm/qemu-arm.c > > index d5ed3eebaf..8cad54c76f 100644 > > --- a/board/emulation/qemu-arm/qemu-arm.c > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > +#include <memalign.h> > > #include <mtd.h> > > > > +#define MTDPARTS_LEN 256 > > +#define MTDIDS_LEN 128 > > + > > +#define DFU_ALT_BUF_LEN SZ_1K > > + > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > +{ > > + struct mtd_info *part; > > + bool first = true; > > + const char *name; > > + int len, partnum = 0; > > + > > + name = mtd->name; > > + len = strlen(buf); > > + > > + if (buf[0] != '\0') > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "mtd %s=", name); > > + > > + list_for_each_entry(part, &mtd->partitions, node) { > > + partnum++; > > + if (!first) > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > ";"); > > + first = false; > > + > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "%s part %d", > > + part->name, partnum); > > + } > > +} > > + > > +void set_dfu_alt_info(char *interface, char *devstr) > > +{ > > + struct mtd_info *mtd; > > + > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > + > > + if (env_get("dfu_alt_info")) > > + return; > > + > > + memset(buf, 0, sizeof(buf)); > > + > > + /* probe all MTD devices */ > > + mtd_probe_devices(); > > + > > + mtd = get_mtd_device_nm("nor0"); > > + if (!IS_ERR_OR_NULL(mtd)) > > + board_get_alt_info(mtd, buf); > > + > > + env_set("dfu_alt_info", buf); > > + printf("dfu_alt_info set\n"); > > +} > > + > > static void board_get_mtdparts(const char *dev, const char *partition, > > char *mtdids, char *mtdparts) > > { > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > index 69ff329434..726f985d35 100644 > > --- a/include/configs/qemu-arm.h > > +++ b/include/configs/qemu-arm.h > > @@ -33,6 +33,7 @@ > > #include <config_distro_bootcmd.h> > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > +#define CONFIG_SET_DFU_ALT_INFO > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > #endif > > > > > >
On 12/7/20 6:42 AM, Sughosh Ganu wrote: > > On Sat, 5 Dec 2020 at 16:01, Heinrich Schuchardt <xypron.glpk@gmx.de > <mailto:xypron.glpk@gmx.de>> wrote: > > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > The dfu framework uses the dfu_alt_info environment variable to get > > information that is needed for performing the firmware update. > Set the > > dfu_alt_info for the platform to reflect the two mtd partitions > > created for the u-boot env and the firmware image. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org > <mailto:sughosh.ganu@linaro.org>> > > I can't see anything QEMU specific in this patch. Why is the code under > board/emulation/? > > > The device that the board wants to use to set the dfu_alt_info to would > be very much board specific. For example, the dfu_alt_info is being set > for the qemu platform for an mtd as the interface, with nor0 as the > device. Different platforms might have different requirements, like the > setting of the variable done for st > platforms(board/st/common/stm32mp_dfu.c). I think this information is > very much platform specific. In the function below you simply collect MTD partitions. Isn't this something that could be reused for other boards? On other boards (e.g. board/samsung/common/misc.c) CONFIG_SET_DFU_ALT_INFO controls if set_dfu_alt_info() exists. Should the same be done for QEMU ARM? Reading through patch 14/14 it is unclear to me how you control to which partition you write TF-A, SPL, U-Boot depending on which of these exists on the MTD device. Wouldn't it make more sense to encode in the capsule to where it will be written? Best regards Heinrich > > -sughosh > > > Best regards > > Heinrich > > > --- > > board/emulation/qemu-arm/qemu-arm.c | 55 > +++++++++++++++++++++++++++++ > > include/configs/qemu-arm.h | 1 + > > 2 files changed, 56 insertions(+) > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > b/board/emulation/qemu-arm/qemu-arm.c > > index d5ed3eebaf..8cad54c76f 100644 > > --- a/board/emulation/qemu-arm/qemu-arm.c > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > +#include <memalign.h> > > #include <mtd.h> > > > > +#define MTDPARTS_LEN 256 > > +#define MTDIDS_LEN 128 > > + > > +#define DFU_ALT_BUF_LEN SZ_1K > > + > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > +{ > > + struct mtd_info *part; > > + bool first = true; > > + const char *name; > > + int len, partnum = 0; > > + > > + name = mtd->name; > > + len = strlen(buf); > > + > > + if (buf[0] != '\0') > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "mtd %s=", name); > > + > > + list_for_each_entry(part, &mtd->partitions, node) { > > + partnum++; > > + if (!first) > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN > - len, ";"); > > + first = false; > > + > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "%s part %d", > > + part->name, partnum); > > + } > > +} > > + > > +void set_dfu_alt_info(char *interface, char *devstr) > > +{ > > + struct mtd_info *mtd; > > + > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > + > > + if (env_get("dfu_alt_info")) > > + return; > > + > > + memset(buf, 0, sizeof(buf)); > > + > > + /* probe all MTD devices */ > > + mtd_probe_devices(); > > + > > + mtd = get_mtd_device_nm("nor0"); > > + if (!IS_ERR_OR_NULL(mtd)) > > + board_get_alt_info(mtd, buf); > > + > > + env_set("dfu_alt_info", buf); > > + printf("dfu_alt_info set\n"); > > +} > > + > > static void board_get_mtdparts(const char *dev, const char > *partition, > > char *mtdids, char *mtdparts) > > { > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > index 69ff329434..726f985d35 100644 > > --- a/include/configs/qemu-arm.h > > +++ b/include/configs/qemu-arm.h > > @@ -33,6 +33,7 @@ > > #include <config_distro_bootcmd.h> > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > +#define CONFIG_SET_DFU_ALT_INFO > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > #endif > > > > >
On Mon, 7 Dec 2020 at 12:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 12/7/20 6:42 AM, Sughosh Ganu wrote: > > > > On Sat, 5 Dec 2020 at 16:01, Heinrich Schuchardt <xypron.glpk@gmx.de > > <mailto:xypron.glpk@gmx.de>> wrote: > > > > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > > The dfu framework uses the dfu_alt_info environment variable to > get > > > information that is needed for performing the firmware update. > > Set the > > > dfu_alt_info for the platform to reflect the two mtd partitions > > > created for the u-boot env and the firmware image. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org > > <mailto:sughosh.ganu@linaro.org>> > > > > I can't see anything QEMU specific in this patch. Why is the code > under > > board/emulation/? > > > > > > The device that the board wants to use to set the dfu_alt_info to would > > be very much board specific. For example, the dfu_alt_info is being set > > for the qemu platform for an mtd as the interface, with nor0 as the > > device. Different platforms might have different requirements, like the > > setting of the variable done for st > > platforms(board/st/common/stm32mp_dfu.c). I think this information is > > very much platform specific. > > In the function below you simply collect MTD partitions. Isn't this > something that could be reused for other boards? > The function probes all mtd devices, but uses only the nor0 device for setting up of the dfu_alt_info variable. The other nor1 device is not used, since, on this platform configuration(non-secure boot), it has the u-boot environment. So which of the mtd devices to use for setting of the dfu_alt_info is very much platform specific. > On other boards (e.g. board/samsung/common/misc.c) > CONFIG_SET_DFU_ALT_INFO controls if set_dfu_alt_info() exists. Should > the same be done for QEMU ARM? > Yes, I can add a check for this on the lines that you mention. Will do. > > Reading through patch 14/14 it is unclear to me how you control to which > partition you write TF-A, SPL, U-Boot depending on which of these exists > on the MTD device. > This gets added in patch 05/14. This sets the mtd partitions that are to be set for the platform. On this particular configuration of the platform(non-secure boot), the partition 1 is used for the u-boot image, and the partition 2 is used for the u-boot environment. > Wouldn't it make more sense to encode in the capsule to where it will be > written? > We have discussed this earlier[1] that there is no way to encode the target device and partition information as part of the capsule, the way the capsule structure is defined by the uefi specification. That is the reason that it was decided to use the dfu backend for the update using the dfu_alt_info variable to indicate the partitions and device to be used for the update. -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2020-May/411236.html > Best regards > > Heinrich > > > > > -sughosh > > > > > > Best regards > > > > Heinrich > > > > > --- > > > board/emulation/qemu-arm/qemu-arm.c | 55 > > +++++++++++++++++++++++++++++ > > > include/configs/qemu-arm.h | 1 + > > > 2 files changed, 56 insertions(+) > > > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > > b/board/emulation/qemu-arm/qemu-arm.c > > > index d5ed3eebaf..8cad54c76f 100644 > > > --- a/board/emulation/qemu-arm/qemu-arm.c > > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > > > +#include <memalign.h> > > > #include <mtd.h> > > > > > > +#define MTDPARTS_LEN 256 > > > +#define MTDIDS_LEN 128 > > > + > > > +#define DFU_ALT_BUF_LEN SZ_1K > > > + > > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > > +{ > > > + struct mtd_info *part; > > > + bool first = true; > > > + const char *name; > > > + int len, partnum = 0; > > > + > > > + name = mtd->name; > > > + len = strlen(buf); > > > + > > > + if (buf[0] != '\0') > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > "&"); > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > + "mtd %s=", name); > > > + > > > + list_for_each_entry(part, &mtd->partitions, node) { > > > + partnum++; > > > + if (!first) > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN > > - len, ";"); > > > + first = false; > > > + > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > + "%s part %d", > > > + part->name, partnum); > > > + } > > > +} > > > + > > > +void set_dfu_alt_info(char *interface, char *devstr) > > > +{ > > > + struct mtd_info *mtd; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > > + > > > + if (env_get("dfu_alt_info")) > > > + return; > > > + > > > + memset(buf, 0, sizeof(buf)); > > > + > > > + /* probe all MTD devices */ > > > + mtd_probe_devices(); > > > + > > > + mtd = get_mtd_device_nm("nor0"); > > > + if (!IS_ERR_OR_NULL(mtd)) > > > + board_get_alt_info(mtd, buf); > > > + > > > + env_set("dfu_alt_info", buf); > > > + printf("dfu_alt_info set\n"); > > > +} > > > + > > > static void board_get_mtdparts(const char *dev, const char > > *partition, > > > char *mtdids, char *mtdparts) > > > { > > > diff --git a/include/configs/qemu-arm.h > b/include/configs/qemu-arm.h > > > index 69ff329434..726f985d35 100644 > > > --- a/include/configs/qemu-arm.h > > > +++ b/include/configs/qemu-arm.h > > > @@ -33,6 +33,7 @@ > > > #include <config_distro_bootcmd.h> > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > +#define CONFIG_SET_DFU_ALT_INFO > > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > > #endif > > > > > > > > > >
On Sat, Dec 05, 2020 at 11:31:49AM +0100, Heinrich Schuchardt wrote: > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > The dfu framework uses the dfu_alt_info environment variable to get > > information that is needed for performing the firmware update. Set the > > dfu_alt_info for the platform to reflect the two mtd partitions > > created for the u-boot env and the firmware image. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > I can't see anything QEMU specific in this patch. Why is the code under > board/emulation/? > > Best regards > > Heinrich > > > --- > > board/emulation/qemu-arm/qemu-arm.c | 55 +++++++++++++++++++++++++++++ > > include/configs/qemu-arm.h | 1 + > > 2 files changed, 56 insertions(+) > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c > > index d5ed3eebaf..8cad54c76f 100644 > > --- a/board/emulation/qemu-arm/qemu-arm.c > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > +#include <memalign.h> > > #include <mtd.h> > > > > +#define MTDPARTS_LEN 256 > > +#define MTDIDS_LEN 128 > > + > > +#define DFU_ALT_BUF_LEN SZ_1K > > + > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > +{ > > + struct mtd_info *part; > > + bool first = true; > > + const char *name; > > + int len, partnum = 0; > > + > > + name = mtd->name; > > + len = strlen(buf); > > + > > + if (buf[0] != '\0') > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "mtd %s=", name); > > + > > + list_for_each_entry(part, &mtd->partitions, node) { > > + partnum++; > > + if (!first) > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, ";"); > > + first = false; > > + > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > + "%s part %d", > > + part->name, partnum); > > + } > > +} > > + > > +void set_dfu_alt_info(char *interface, char *devstr) > > +{ > > + struct mtd_info *mtd; > > + > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > + > > + if (env_get("dfu_alt_info")) > > + return; > > + > > + memset(buf, 0, sizeof(buf)); > > + > > + /* probe all MTD devices */ > > + mtd_probe_devices(); > > + > > + mtd = get_mtd_device_nm("nor0"); > > + if (!IS_ERR_OR_NULL(mtd)) > > + board_get_alt_info(mtd, buf); > > + > > + env_set("dfu_alt_info", buf); > > + printf("dfu_alt_info set\n"); > > +} > > + > > static void board_get_mtdparts(const char *dev, const char *partition, > > char *mtdids, char *mtdparts) > > { > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > index 69ff329434..726f985d35 100644 > > --- a/include/configs/qemu-arm.h > > +++ b/include/configs/qemu-arm.h > > @@ -33,6 +33,7 @@ > > #include <config_distro_bootcmd.h> > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > +#define CONFIG_SET_DFU_ALT_INFO > > #define CONFIG_SYS_MTDPARTS_RUNTIME First, CONFIG_SET_DFU_ALT_INFO is in Kconfig and needs to be enabled that way. Second, typically we just set the information in the environment part of the header. This is especially true if in both this case and the previous patch to add mtdparts, we don't really have this being dynamic? Or are we really expecting / supporting arbitrary sized flash as this is qemu? Thanks. -- Tom
On Tue, 8 Dec 2020 at 00:17, Tom Rini <trini@konsulko.com> wrote: > On Sat, Dec 05, 2020 at 11:31:49AM +0100, Heinrich Schuchardt wrote: > > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > > The dfu framework uses the dfu_alt_info environment variable to get > > > information that is needed for performing the firmware update. Set the > > > dfu_alt_info for the platform to reflect the two mtd partitions > > > created for the u-boot env and the firmware image. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > I can't see anything QEMU specific in this patch. Why is the code under > > board/emulation/? > > > > Best regards > > > > Heinrich > > > > > --- > > > board/emulation/qemu-arm/qemu-arm.c | 55 > +++++++++++++++++++++++++++++ > > > include/configs/qemu-arm.h | 1 + > > > 2 files changed, 56 insertions(+) > > > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > b/board/emulation/qemu-arm/qemu-arm.c > > > index d5ed3eebaf..8cad54c76f 100644 > > > --- a/board/emulation/qemu-arm/qemu-arm.c > > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > > > +#include <memalign.h> > > > #include <mtd.h> > > > > > > +#define MTDPARTS_LEN 256 > > > +#define MTDIDS_LEN 128 > > > + > > > +#define DFU_ALT_BUF_LEN SZ_1K > > > + > > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > > +{ > > > + struct mtd_info *part; > > > + bool first = true; > > > + const char *name; > > > + int len, partnum = 0; > > > + > > > + name = mtd->name; > > > + len = strlen(buf); > > > + > > > + if (buf[0] != '\0') > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > + "mtd %s=", name); > > > + > > > + list_for_each_entry(part, &mtd->partitions, node) { > > > + partnum++; > > > + if (!first) > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > ";"); > > > + first = false; > > > + > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > + "%s part %d", > > > + part->name, partnum); > > > + } > > > +} > > > + > > > +void set_dfu_alt_info(char *interface, char *devstr) > > > +{ > > > + struct mtd_info *mtd; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > > + > > > + if (env_get("dfu_alt_info")) > > > + return; > > > + > > > + memset(buf, 0, sizeof(buf)); > > > + > > > + /* probe all MTD devices */ > > > + mtd_probe_devices(); > > > + > > > + mtd = get_mtd_device_nm("nor0"); > > > + if (!IS_ERR_OR_NULL(mtd)) > > > + board_get_alt_info(mtd, buf); > > > + > > > + env_set("dfu_alt_info", buf); > > > + printf("dfu_alt_info set\n"); > > > +} > > > + > > > static void board_get_mtdparts(const char *dev, const char > *partition, > > > char *mtdids, char *mtdparts) > > > { > > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > > index 69ff329434..726f985d35 100644 > > > --- a/include/configs/qemu-arm.h > > > +++ b/include/configs/qemu-arm.h > > > @@ -33,6 +33,7 @@ > > > #include <config_distro_bootcmd.h> > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > +#define CONFIG_SET_DFU_ALT_INFO > > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > First, CONFIG_SET_DFU_ALT_INFO is in Kconfig and needs to be enabled > that way. Will change in the next version. > Second, typically we just set the information in the > environment part of the header. This is especially true if in both this > case and the previous patch to add mtdparts, we don't really have this > being dynamic? Or are we really expecting / supporting arbitrary sized > flash as this is qemu? Thanks. > I am not sure I understand this. One thing that can be done is to move the setting of the mtd partitions to the Kconfig file, on similar lines to what is being done for the st platforms, e.g MTDPARTS_NOR0_TEE. I can move the mtd partition definitions to the Kconfig file if that is what you are asking for. -sughosh
On Tue, Dec 08, 2020 at 10:48:57AM +0530, Sughosh Ganu wrote: > On Tue, 8 Dec 2020 at 00:17, Tom Rini <trini@konsulko.com> wrote: > > > On Sat, Dec 05, 2020 at 11:31:49AM +0100, Heinrich Schuchardt wrote: > > > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > > > The dfu framework uses the dfu_alt_info environment variable to get > > > > information that is needed for performing the firmware update. Set the > > > > dfu_alt_info for the platform to reflect the two mtd partitions > > > > created for the u-boot env and the firmware image. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > I can't see anything QEMU specific in this patch. Why is the code under > > > board/emulation/? > > > > > > Best regards > > > > > > Heinrich > > > > > > > --- > > > > board/emulation/qemu-arm/qemu-arm.c | 55 > > +++++++++++++++++++++++++++++ > > > > include/configs/qemu-arm.h | 1 + > > > > 2 files changed, 56 insertions(+) > > > > > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > > b/board/emulation/qemu-arm/qemu-arm.c > > > > index d5ed3eebaf..8cad54c76f 100644 > > > > --- a/board/emulation/qemu-arm/qemu-arm.c > > > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > > > > > +#include <memalign.h> > > > > #include <mtd.h> > > > > > > > > +#define MTDPARTS_LEN 256 > > > > +#define MTDIDS_LEN 128 > > > > + > > > > +#define DFU_ALT_BUF_LEN SZ_1K > > > > + > > > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > > > +{ > > > > + struct mtd_info *part; > > > > + bool first = true; > > > > + const char *name; > > > > + int len, partnum = 0; > > > > + > > > > + name = mtd->name; > > > > + len = strlen(buf); > > > > + > > > > + if (buf[0] != '\0') > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > > + "mtd %s=", name); > > > > + > > > > + list_for_each_entry(part, &mtd->partitions, node) { > > > > + partnum++; > > > > + if (!first) > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > ";"); > > > > + first = false; > > > > + > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > > + "%s part %d", > > > > + part->name, partnum); > > > > + } > > > > +} > > > > + > > > > +void set_dfu_alt_info(char *interface, char *devstr) > > > > +{ > > > > + struct mtd_info *mtd; > > > > + > > > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > > > + > > > > + if (env_get("dfu_alt_info")) > > > > + return; > > > > + > > > > + memset(buf, 0, sizeof(buf)); > > > > + > > > > + /* probe all MTD devices */ > > > > + mtd_probe_devices(); > > > > + > > > > + mtd = get_mtd_device_nm("nor0"); > > > > + if (!IS_ERR_OR_NULL(mtd)) > > > > + board_get_alt_info(mtd, buf); > > > > + > > > > + env_set("dfu_alt_info", buf); > > > > + printf("dfu_alt_info set\n"); > > > > +} > > > > + > > > > static void board_get_mtdparts(const char *dev, const char > > *partition, > > > > char *mtdids, char *mtdparts) > > > > { > > > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > > > index 69ff329434..726f985d35 100644 > > > > --- a/include/configs/qemu-arm.h > > > > +++ b/include/configs/qemu-arm.h > > > > @@ -33,6 +33,7 @@ > > > > #include <config_distro_bootcmd.h> > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > +#define CONFIG_SET_DFU_ALT_INFO > > > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > > > First, CONFIG_SET_DFU_ALT_INFO is in Kconfig and needs to be enabled > > that way. > > > Will change in the next version. > > > > Second, typically we just set the information in the > > environment part of the header. This is especially true if in both this > > case and the previous patch to add mtdparts, we don't really have this > > being dynamic? Or are we really expecting / supporting arbitrary sized > > flash as this is qemu? Thanks. > > > > I am not sure I understand this. One thing that can be done is to move the > setting of the mtd partitions to the Kconfig file, on similar lines to what > is being done for the st platforms, e.g MTDPARTS_NOR0_TEE. I can move the > mtd partition definitions to the Kconfig file if that is what you are > asking for. Environment manipulation via C code is possible, but discouraged. What can be set via Kconfig should be set that way, and what can be put in the environment part of the header should be done that way. Does that help clarify? -- Tom
On Tue, 8 Dec 2020 at 17:50, Tom Rini <trini@konsulko.com> wrote: > On Tue, Dec 08, 2020 at 10:48:57AM +0530, Sughosh Ganu wrote: > > On Tue, 8 Dec 2020 at 00:17, Tom Rini <trini@konsulko.com> wrote: > > > > > On Sat, Dec 05, 2020 at 11:31:49AM +0100, Heinrich Schuchardt wrote: > > > > On 11/26/20 7:41 PM, Sughosh Ganu wrote: > > > > > The dfu framework uses the dfu_alt_info environment variable to get > > > > > information that is needed for performing the firmware update. Set > the > > > > > dfu_alt_info for the platform to reflect the two mtd partitions > > > > > created for the u-boot env and the firmware image. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > I can't see anything QEMU specific in this patch. Why is the code > under > > > > board/emulation/? > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > --- > > > > > board/emulation/qemu-arm/qemu-arm.c | 55 > > > +++++++++++++++++++++++++++++ > > > > > include/configs/qemu-arm.h | 1 + > > > > > 2 files changed, 56 insertions(+) > > > > > > > > > > diff --git a/board/emulation/qemu-arm/qemu-arm.c > > > b/board/emulation/qemu-arm/qemu-arm.c > > > > > index d5ed3eebaf..8cad54c76f 100644 > > > > > --- a/board/emulation/qemu-arm/qemu-arm.c > > > > > +++ b/board/emulation/qemu-arm/qemu-arm.c > > > > > @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) > > > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > > > > > > > +#include <memalign.h> > > > > > #include <mtd.h> > > > > > > > > > > +#define MTDPARTS_LEN 256 > > > > > +#define MTDIDS_LEN 128 > > > > > + > > > > > +#define DFU_ALT_BUF_LEN SZ_1K > > > > > + > > > > > +static void board_get_alt_info(struct mtd_info *mtd, char *buf) > > > > > +{ > > > > > + struct mtd_info *part; > > > > > + bool first = true; > > > > > + const char *name; > > > > > + int len, partnum = 0; > > > > > + > > > > > + name = mtd->name; > > > > > + len = strlen(buf); > > > > > + > > > > > + if (buf[0] != '\0') > > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); > > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > > > + "mtd %s=", name); > > > > > + > > > > > + list_for_each_entry(part, &mtd->partitions, node) { > > > > > + partnum++; > > > > > + if (!first) > > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - > len, > > > ";"); > > > > > + first = false; > > > > > + > > > > > + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, > > > > > + "%s part %d", > > > > > + part->name, partnum); > > > > > + } > > > > > +} > > > > > + > > > > > +void set_dfu_alt_info(char *interface, char *devstr) > > > > > +{ > > > > > + struct mtd_info *mtd; > > > > > + > > > > > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); > > > > > + > > > > > + if (env_get("dfu_alt_info")) > > > > > + return; > > > > > + > > > > > + memset(buf, 0, sizeof(buf)); > > > > > + > > > > > + /* probe all MTD devices */ > > > > > + mtd_probe_devices(); > > > > > + > > > > > + mtd = get_mtd_device_nm("nor0"); > > > > > + if (!IS_ERR_OR_NULL(mtd)) > > > > > + board_get_alt_info(mtd, buf); > > > > > + > > > > > + env_set("dfu_alt_info", buf); > > > > > + printf("dfu_alt_info set\n"); > > > > > +} > > > > > + > > > > > static void board_get_mtdparts(const char *dev, const char > > > *partition, > > > > > char *mtdids, char *mtdparts) > > > > > { > > > > > diff --git a/include/configs/qemu-arm.h > b/include/configs/qemu-arm.h > > > > > index 69ff329434..726f985d35 100644 > > > > > --- a/include/configs/qemu-arm.h > > > > > +++ b/include/configs/qemu-arm.h > > > > > @@ -33,6 +33,7 @@ > > > > > #include <config_distro_bootcmd.h> > > > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) > > > > > +#define CONFIG_SET_DFU_ALT_INFO > > > > > #define CONFIG_SYS_MTDPARTS_RUNTIME > > > > > > First, CONFIG_SET_DFU_ALT_INFO is in Kconfig and needs to be enabled > > > that way. > > > > > > Will change in the next version. > > > > > > > Second, typically we just set the information in the > > > environment part of the header. This is especially true if in both > this > > > case and the previous patch to add mtdparts, we don't really have this > > > being dynamic? Or are we really expecting / supporting arbitrary sized > > > flash as this is qemu? Thanks. > > > > > > > I am not sure I understand this. One thing that can be done is to move > the > > setting of the mtd partitions to the Kconfig file, on similar lines to > what > > is being done for the st platforms, e.g MTDPARTS_NOR0_TEE. I can move the > > mtd partition definitions to the Kconfig file if that is what you are > > asking for. > > Environment manipulation via C code is possible, but discouraged. What > can be set via Kconfig should be set that way, and what can be put in > the environment part of the header should be done that way. Does that > help clarify? > Yes, that does clarify. I will move the settings of the partitions used for mtdparts in the Kconfig, similar to the way it is being done for the st platforms. Thanks. -sughosh
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index d5ed3eebaf..8cad54c76f 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -200,8 +200,63 @@ void flash_write32(u32 value, void *addr) #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) +#include <memalign.h> #include <mtd.h> +#define MTDPARTS_LEN 256 +#define MTDIDS_LEN 128 + +#define DFU_ALT_BUF_LEN SZ_1K + +static void board_get_alt_info(struct mtd_info *mtd, char *buf) +{ + struct mtd_info *part; + bool first = true; + const char *name; + int len, partnum = 0; + + name = mtd->name; + len = strlen(buf); + + if (buf[0] != '\0') + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, "&"); + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, + "mtd %s=", name); + + list_for_each_entry(part, &mtd->partitions, node) { + partnum++; + if (!first) + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, ";"); + first = false; + + len += snprintf(buf + len, DFU_ALT_BUF_LEN - len, + "%s part %d", + part->name, partnum); + } +} + +void set_dfu_alt_info(char *interface, char *devstr) +{ + struct mtd_info *mtd; + + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN); + + if (env_get("dfu_alt_info")) + return; + + memset(buf, 0, sizeof(buf)); + + /* probe all MTD devices */ + mtd_probe_devices(); + + mtd = get_mtd_device_nm("nor0"); + if (!IS_ERR_OR_NULL(mtd)) + board_get_alt_info(mtd, buf); + + env_set("dfu_alt_info", buf); + printf("dfu_alt_info set\n"); +} + static void board_get_mtdparts(const char *dev, const char *partition, char *mtdids, char *mtdparts) { diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h index 69ff329434..726f985d35 100644 --- a/include/configs/qemu-arm.h +++ b/include/configs/qemu-arm.h @@ -33,6 +33,7 @@ #include <config_distro_bootcmd.h> #if defined(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) +#define CONFIG_SET_DFU_ALT_INFO #define CONFIG_SYS_MTDPARTS_RUNTIME #endif
The dfu framework uses the dfu_alt_info environment variable to get information that is needed for performing the firmware update. Set the dfu_alt_info for the platform to reflect the two mtd partitions created for the u-boot env and the firmware image. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- board/emulation/qemu-arm/qemu-arm.c | 55 +++++++++++++++++++++++++++++ include/configs/qemu-arm.h | 1 + 2 files changed, 56 insertions(+) -- 2.17.1