On Tue, 8 Dec 2020 at 17:50, Tom Rini <tr...@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 <tr...@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.g...@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
