diff mbox series

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

Message ID 20230109010642.578484-1-jaswinder.singh@linaro.org
State New
Headers show
Series FWU: Add support for mtd backed feature on DeveloperBox | expand

Commit Message

Jassi Brar Jan. 9, 2023, 1:06 a.m. UTC
From: Sughosh Ganu <sughosh.ganu@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@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@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

Comments

Sughosh Ganu Jan. 13, 2023, 10:41 a.m. UTC | #1
On Mon, 9 Jan 2023 at 06:36, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> From: Sughosh Ganu <sughosh.ganu@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@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

This patch is authored by Masami Hiramatsu. Please attribute it to him.

-sughosh
Michal Simek Jan. 18, 2023, 2:24 p.m. UTC | #2
On 1/9/23 02:06, Jassi Brar wrote:
> From: Sughosh Ganu <sughosh.ganu@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@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@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.

> +
> +#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.

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

./scripts/kernel-doc -man -v drivers/fwu-mdata/raw_mtd.c 1>/dev/null
drivers/fwu-mdata/raw_mtd.c:122: info: Scanning doc for fwu_mdata_mtd_of_to_plat
drivers/fwu-mdata/raw_mtd.c:125: warning: Function parameter or member 'dev' not 
described in 'fwu_mdata_mtd_of_to_plat'
drivers/fwu-mdata/raw_mtd.c:125: warning: No description found for return value 
of 'fwu_mdata_mtd_of_to_plat'



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

> +	}
> +
> +	/* 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@500000 {
  80                                         label = "Ex-OPTEE";
  81                                         reg = <0x500000 0x200000>;
  82                                 };

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


> +
> +	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


> +}
> +
> +static struct fwu_mdata_ops fwu_mtd_ops = {
> +	.read_mdata = fwu_mtd_read_mdata,
> +	.write_mdata = fwu_mtd_write_mdata,
> +};
> +
> +static const struct udevice_id fwu_mdata_ids[] = {
> +	{ .compatible = "u-boot,fwu-mdata-mtd" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(fwu_mdata_mtd) = {
> +	.name		= "fwu-mdata-mtd",
> +	.id		= UCLASS_FWU_MDATA,
> +	.of_match	= fwu_mdata_ids,
> +	.ops		= &fwu_mtd_ops,
> +	.probe		= fwu_mdata_mtd_probe,
> +	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
> +	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
> +};

Thanks,
Michal
Jassi Brar Feb. 5, 2023, 4:09 a.m. UTC | #3
On Wed, 18 Jan 2023 at 08:24, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 1/9/23 02:06, Jassi Brar wrote:
> > From: Sughosh Ganu <sughosh.ganu@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@linaro.org>
> > Signed-off-by: Jassi Brar <jaswinder.singh@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@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
Jassi Brar Feb. 28, 2023, 12:58 a.m. UTC | #4
On Sat, 4 Feb 2023 at 22:09, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> >
> > +               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@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.
>
I changed the bindings to not require any changes to any non-FWU subsystem.
Now every bit of info is contained in 'fwu-mdata' node.
   https://lore.kernel.org/u-boot/20230228005218.1635781-1-jassisinghbrar@gmail.com/T/#u

Thanks.
diff mbox series

Patch

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>
+
+#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
+
+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
+ */
+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;
+	}
+
+	/* 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;
+
+	return 0;
+}
+
+static int fwu_mdata_mtd_probe(struct udevice *dev)
+{
+	/* Ensure the metadata can be read. */
+	return fwu_get_mdata(NULL);
+}
+
+static struct fwu_mdata_ops fwu_mtd_ops = {
+	.read_mdata = fwu_mtd_read_mdata,
+	.write_mdata = fwu_mtd_write_mdata,
+};
+
+static const struct udevice_id fwu_mdata_ids[] = {
+	{ .compatible = "u-boot,fwu-mdata-mtd" },
+	{ }
+};
+
+U_BOOT_DRIVER(fwu_mdata_mtd) = {
+	.name		= "fwu-mdata-mtd",
+	.id		= UCLASS_FWU_MDATA,
+	.of_match	= fwu_mdata_ids,
+	.ops		= &fwu_mtd_ops,
+	.probe		= fwu_mdata_mtd_probe,
+	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
+	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
+};