[PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

Jassi Brar jaswinder.singh at linaro.org
Sun Feb 5 05:09:45 CET 2023


On Wed, 18 Jan 2023 at 08:24, Michal Simek <michal.simek at amd.com> wrote:
>
>
>
> On 1/9/23 02:06, Jassi Brar wrote:
> > From: Sughosh Ganu <sughosh.ganu at linaro.org>
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > region. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a raw
> > MTD region.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > Signed-off-by: Jassi Brar <jaswinder.singh at linaro.org>
> > ---
> >   drivers/fwu-mdata/Kconfig   |  15 +++
> >   drivers/fwu-mdata/Makefile  |   1 +
> >   drivers/fwu-mdata/raw_mtd.c | 201 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 217 insertions(+)
> >   create mode 100644 drivers/fwu-mdata/raw_mtd.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index 36c4479a59..42736a5e43 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -6,6 +6,11 @@ config FWU_MDATA
> >         FWU Metadata partitions reside on the same storage device
> >         which contains the other FWU updatable firmware images.
> >
> > +choice
> > +     prompt "Storage Layout Scheme"
> > +     depends on FWU_MDATA
> > +     default FWU_MDATA_GPT_BLK
> > +
> >   config FWU_MDATA_GPT_BLK
> >       bool "FWU Metadata access for GPT partitioned Block devices"
> >       select PARTITION_TYPE_GUID
> > @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
> >       help
> >         Enable support for accessing FWU Metadata on GPT partitioned
> >         block devices.
> > +
> > +config FWU_MDATA_MTD
> > +     bool "Raw MTD devices"
> > +     depends on MTD
> > +     help
> > +       Enable support for accessing FWU Metadata on non-partitioned
> > +       (or non-GPT partitioned, e.g. partition nodes in devicetree)
> > +       MTD devices.
> > +
> > +endchoice
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index 3fee64c10c..06c49747ba 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -6,3 +6,4 @@
> >
> >   obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
> >   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
> > +obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 0000000000..edd28b4525
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_FWU_MDATA
> > +
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <memalign.h>
> > +#include <flash.h>
>
> sort them.
>
actually flash.h is not required.


> > +
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +
> > +/* Internal helper structure to move data around */
> > +struct fwu_mdata_mtd_priv {
> > +     struct mtd_info *mtd;
> > +     u32 pri_offset;
> > +     u32 sec_offset;
> > +};
> > +
> > +enum fwu_mtd_op {
> > +     FWU_MTD_READ,
> > +     FWU_MTD_WRITE,
> > +};
> > +
> > +#define FWU_MDATA_PRIMARY    true
> > +#define FWU_MDATA_SECONDARY  false
>
> Completely unused.
>
yes, removed.


> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > +     return !do_div(size, mtd->erasesize);
> > +}
> > +
> > +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> > +                    enum fwu_mtd_op op)
> > +{
> > +     struct mtd_oob_ops io_op ={};
> > +     u64 lock_offs, lock_len;
> > +     size_t len;
> > +     void *buf;
> > +     int ret;
> > +
> > +     if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> > +             log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
> > +             return -EINVAL;
> > +     }
> > +
> > +     lock_offs = offs;
> > +     /* This will expand erase size to align with the block size */
> > +     lock_len = round_up(size, mtd->erasesize);
> > +
> > +     ret = mtd_unlock(mtd, lock_offs, lock_len);
> > +     if (ret && ret != -EOPNOTSUPP)
> > +             return ret;
> > +
> > +     if (op == FWU_MTD_WRITE) {
> > +             struct erase_info erase_op = {};
> > +
> > +             erase_op.mtd = mtd;
> > +             erase_op.addr = lock_offs;
> > +             erase_op.len = lock_len;
> > +             erase_op.scrub = 0;
> > +
> > +             ret = mtd_erase(mtd, &erase_op);
> > +             if (ret)
> > +                     goto lock;
> > +     }
> > +
> > +     /* Also, expand the write size to align with the write size */
> > +     len = round_up(size, mtd->writesize);
> > +
> > +     buf = memalign(ARCH_DMA_MINALIGN, len);
> > +     if (!buf) {
> > +             ret = -ENOMEM;
> > +             goto lock;
> > +     }
> > +     memset(buf, 0xff, len);
> > +
> > +     io_op.mode = MTD_OPS_AUTO_OOB;
> > +     io_op.len = len;
> > +     io_op.ooblen = 0;
> > +     io_op.datbuf = buf;
> > +     io_op.oobbuf = NULL;
> > +
> > +     if (op == FWU_MTD_WRITE) {
> > +             memcpy(buf, data, size);
> > +             ret = mtd_write_oob(mtd, offs, &io_op);
> > +     } else {
> > +             ret = mtd_read_oob(mtd, offs, &io_op);
> > +             if (!ret)
> > +                     memcpy(data, buf, size);
> > +     }
> > +     free(buf);
> > +
> > +lock:
> > +     mtd_lock(mtd, lock_offs, lock_len);
> > +
> > +     return ret;
> > +}
> > +
> > +static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     struct mtd_info *mtd = mtd_priv->mtd;
> > +     u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> > +
> > +     return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
> > +}
> > +
> > +static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     struct mtd_info *mtd = mtd_priv->mtd;
> > +     u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> > +
> > +     return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> > +}
> > +
> > +/**
> > + * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
> > + */
>
> Fix kernel-doc format.
>
I think we don't even need to document the static .of_to_plat() callback


>
> > +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     const fdt32_t *phandle_p = NULL;
> > +     struct udevice *mtd_dev;
> > +     struct mtd_info *mtd;
> > +     int ret, size;
> > +     u32 phandle;
> > +
> > +     /* Find the FWU mdata storage device */
> > +     phandle_p = ofnode_get_property(dev_ofnode(dev),
> > +                                     "fwu-mdata-store", &size);
> > +     if (!phandle_p) {
> > +             log_err("FWU meta data store not defined in device-tree\n");
> > +             return -ENOENT;
> > +     }
> > +
> > +     phandle = fdt32_to_cpu(*phandle_p);
> > +
> > +     ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> > +                                                                       &mtd_dev);
> > +     if (ret) {
> > +             log_err("FWU: failed to get mtd device\n");
> > +             return ret;
> > +     }
> > +
> > +     mtd_probe_devices();
> > +
> > +     mtd_for_each_device(mtd) {
> > +             if (mtd->dev == mtd_dev) {
> > +                     mtd_priv->mtd = mtd;
> > +                     log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
> > +                     break;
> > +             }
> > +     }
> > +     if (!mtd_priv->mtd) {
> > +             log_err("Failed to find mtd device by fwu-mdata-store\n");
> > +             return -ENOENT;
>
> -ENODEV is likely better.
>
ok.


> > +     }
> > +
> > +     /* Get the offset of primary and seconday mdata */
> > +     ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,
> > +                                 &mtd_priv->pri_offset);
> > +     if (ret)
> > +             return ret;
> > +     ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,
> > +                                 &mtd_priv->sec_offset);
> > +     if (ret)
> > +             return ret;
>
> I am not getting usage of these offsets.
> First of all in DT patch you are using
>
> +               fwu-mdata {
> +                       compatible = "u-boot,fwu-mdata-mtd";
> +                       fwu-mdata-store = <&spi_flash>;
> +                       mdata-offsets = <0x500000 0x530000>;
> +               };
>
> which is based on DT going to location which is already labelled.
>
>   79                                 partition at 500000 {
>   80                                         label = "Ex-OPTEE";
>   81                                         reg = <0x500000 0x200000>;
>   82                                 };
>
The 'ex-optee' is actually unused so we never faced any issue. But
yes, this is inconsistent.

> I don't know what this space is used for but the whole code around is using MTD
> partitions and it's infrastructure and this is using RAW access without MTD.
>
> Why not to create separate partitions just for storing metadata?
> And also identify them like that.
>
> Or just switch it to complete RAW mode without MTD and then offsets can be used
> (but I expect with different dt description).
>
The design predates my involvement in fwu. I guess the reason was to
have a mechanism similar to GPT based implementation which uses a
similar fwu-mdata {} node structure.  It does make sense to have
dedicated partitions for primary and  secondary meta-data, identified
by uuid (like Banks) or standard labels. But may be Sughosh/Ilias know
why the current approach was chosen.


>
> > +
> > +     return 0;
> > +}
> > +
> > +static int fwu_mdata_mtd_probe(struct udevice *dev)
> > +{
> > +     /* Ensure the metadata can be read. */
> > +     return fwu_get_mdata(NULL);
>
> Likely I am not getting how this is used but I would expect that ops->get_mdata
> function is going to befined here too.
>
> And also ops->update_mdata
>
This just caches the meta-data after checking/fixing for its
integrity. This way we avoid reading from mtd for each mdata fetch.

thanks


More information about the U-Boot mailing list