diff mbox series

[v4,1/6] FWU: Add FWU metadata access driver for MTD storage regions

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

Commit Message

Jassi Brar March 27, 2023, 9:15 p.m. UTC
From: Masami Hiramatsu <masami.hiramatsu@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: Masami Hiramatsu <masami.hiramatsu@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 | 272 ++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/fwu-mdata/raw_mtd.c

Comments

Michal Simek March 29, 2023, 11:59 a.m. UTC | #1
On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> From: Masami Hiramatsu <masami.hiramatsu@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: Masami Hiramatsu <masami.hiramatsu@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 | 272 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 288 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..4b1a10073a
> --- /dev/null
> +++ b/drivers/fwu-mdata/raw_mtd.c
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0+

Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?

> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#define LOG_CATEGORY UCLASS_FWU_MDATA
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.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;
> +	char pri_label[50];
> +	char sec_label[50];
> +	u32 pri_offset;
> +	u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> +	FWU_MTD_READ,
> +	FWU_MTD_WRITE,
> +};
> +
> +extern struct fwu_mtd_image_info fwu_mtd_images[];

if there is a need to share it. It should go to header.

And fwu_mtd_images[] is not defined when only this patch is applied.
It means order of patches is not right. 1/6 and 2/6 should be swapped


> +
> +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);
> +}
> +
> +static int flash_partition_offset(struct udevice *dev, const char *part_name, fdt_addr_t *offset)
> +{
> +	ofnode node, parts_node;
> +	fdt_addr_t size = 0;
> +
> +	parts_node = ofnode_by_compatible(dev_ofnode(dev), "fixed-partitions");
> +	node = ofnode_by_prop_value(parts_node, "label", part_name, strlen(part_name) + 1);
> +	if (!ofnode_valid(node)) {
> +		log_err("Warning: Failed to find partition by label <%s>\n", part_name);
> +		return -ENOENT;
> +	}
> +
> +	*offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
> +
> +	return (int)size;
> +}
> +
> +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;
> +	const char *label;
> +	fdt_addr_t offset;
> +	int ret, size;
> +	u32 phandle;
> +	ofnode bank;
> +	int off_img;
> +
> +	/* 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 -ENODEV;
> +	}
> +
> +	/* Get the offset of primary and seconday mdata */


typo


> +	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0, &label);
> +	if (ret)
> +		return ret;
> +	strcpy(mtd_priv->pri_label, label);
> +
> +	ret = flash_partition_offset(mtd_dev, mtd_priv->pri_label, &offset);
> +	if (ret <= 0)
> +		return ret;
> +	mtd_priv->pri_offset = offset;
> +
> +	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 1, &label);
> +	if (ret)
> +		return ret;
> +	strcpy(mtd_priv->sec_label, label);
> +
> +	ret = flash_partition_offset(mtd_dev, mtd_priv->sec_label, &offset);
> +	if (ret <= 0)
> +		return ret;
> +	mtd_priv->sec_offset = offset;
> +
> +	off_img = 0;
> +
> +	ofnode_for_each_subnode(bank, dev_ofnode(dev)) {
> +		int bank_num, bank_offset, bank_size;
> +		const char *bank_name;
> +		ofnode image;
> +
> +		ofnode_read_u32(bank, "id", &bank_num);
> +		bank_name = ofnode_read_string(bank, "label");
> +		bank_size = flash_partition_offset(mtd_dev, bank_name, &offset);
> +		if (bank_size <= 0)
> +			return bank_size;
> +		bank_offset = offset;
> +		log_debug("Bank%d: %s [0x%x - 0x%x]\n",
> +			  bank_num, bank_name, bank_offset, bank_offset + bank_size);
> +
> +		ofnode_for_each_subnode(image, bank) {
> +			int image_num, image_offset, image_size;
> +			const char *uuid;
> +
> +			if (off_img == CONFIG_FWU_NUM_BANKS *
> +						CONFIG_FWU_NUM_IMAGES_PER_BANK) {
> +				log_err("DT provides images more than configured!\n");
> +				break;
> +			}
> +
> +			uuid = ofnode_read_string(image, "uuid");
> +			ofnode_read_u32(image, "id", &image_num);
> +			ofnode_read_u32(image, "offset", &image_offset);
> +			ofnode_read_u32(image, "size", &image_size);
> +
> +			fwu_mtd_images[off_img].start = bank_offset + image_offset;
> +			fwu_mtd_images[off_img].size = image_size;
> +			fwu_mtd_images[off_img].bank_num = bank_num;
> +			fwu_mtd_images[off_img].image_num = image_num;
> +			strcpy(fwu_mtd_images[off_img].uuidbuf, uuid);
> +			log_debug("\tImage%d: %s @0x%x\n\n",
> +				  image_num, uuid, bank_offset + image_offset);
> +			off_img++;
> +		}
> +	}
> +
> +	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" },
> +	{ }
> +};

As I said this DT binding should be approved first to make sure that we don't 
need to fix DT binding in future. Just simply do it right from the begining.

Thanks,
Michal
Jassi Brar April 10, 2023, 3:56 a.m. UTC | #2
On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:

> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 0000000000..4b1a10073a
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,272 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>
just c&p. though isn't that the same as GPL-2.0-or-later ?

.......
> > +
> > +extern struct fwu_mtd_image_info fwu_mtd_images[];
>
> if there is a need to share it. It should go to header.
>
include/fwu,h  would be the header. Which is supposed to be very
global, and doesn't seem very appropriate to mention private data
shared between a driver and helper library.
If people still insist, I will make the change.

> And fwu_mtd_images[] is not defined when only this patch is applied.
> It means order of patches is not right. 1/6 and 2/6 should be swapped
>
I think 1 and 2 should be merged rather.


......
> > +     if (!mtd_priv->mtd) {
> > +             log_err("Failed to find mtd device by fwu-mdata-store\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* Get the offset of primary and seconday mdata */
>
>
> typo
>
ok

......
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > +     { .compatible = "u-boot,fwu-mdata-mtd" },
> > +     { }
> > +};
>
> As I said this DT binding should be approved first to make sure that we don't
> need to fix DT binding in future. Just simply do it right from the begining.
>
Yes, I will cc Rob in the next submission (I only forgot last time).
However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
I am not sure if there is any reason for the fwu node to even be in
the dts for kernel. But sure it is good to have it eyeballed by the DT
gods.

cheers.
Michal Simek April 14, 2023, 1:56 p.m. UTC | #3
On 4/10/23 05:56, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> 
>>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
>>> new file mode 100644
>>> index 0000000000..4b1a10073a
>>> --- /dev/null
>>> +++ b/drivers/fwu-mdata/raw_mtd.c
>>> @@ -0,0 +1,272 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>
>> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>>
> just c&p. though isn't that the same as GPL-2.0-or-later ?

license choice is up to you. We normally use just gpl-2.0.

> 
> .......
>>> +
>>> +extern struct fwu_mtd_image_info fwu_mtd_images[];
>>
>> if there is a need to share it. It should go to header.
>>
> include/fwu,h  would be the header. Which is supposed to be very
> global, and doesn't seem very appropriate to mention private data
> shared between a driver and helper library.
> If people still insist, I will make the change.
> 
>> And fwu_mtd_images[] is not defined when only this patch is applied.
>> It means order of patches is not right. 1/6 and 2/6 should be swapped
>>
> I think 1 and 2 should be merged rather.

no issue with it.

> 
> ......
>>> +     if (!mtd_priv->mtd) {
>>> +             log_err("Failed to find mtd device by fwu-mdata-store\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     /* Get the offset of primary and seconday mdata */
>>
>>
>> typo
>>
> ok
> 
> ......
>>> +
>>> +static const struct udevice_id fwu_mdata_ids[] = {
>>> +     { .compatible = "u-boot,fwu-mdata-mtd" },
>>> +     { }
>>> +};
>>
>> As I said this DT binding should be approved first to make sure that we don't
>> need to fix DT binding in future. Just simply do it right from the begining.
>>
> Yes, I will cc Rob in the next submission (I only forgot last time).
> However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> I am not sure if there is any reason for the fwu node to even be in
> the dts for kernel. But sure it is good to have it eyeballed by the DT
> gods.

It doesn't really go to kernel.
Simon pushed options node directly to dt-schema
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
And that would be also location for this node too.

Thanks,
Michal
Jassi Brar April 14, 2023, 3:09 p.m. UTC | #4
On Fri, Apr 14, 2023 at 8:56 AM Michal Simek <michal.simek@amd.com> wrote:
> On 4/10/23 05:56, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> >> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> >
> >>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> >>> new file mode 100644
> >>> index 0000000000..4b1a10073a
> >>> --- /dev/null
> >>> +++ b/drivers/fwu-mdata/raw_mtd.c
> >>> @@ -0,0 +1,272 @@
> >>> +// SPDX-License-Identifier: GPL-2.0+
> >>
> >> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
> >>
> > just c&p. though isn't that the same as GPL-2.0-or-later ?
>
> license choice is up to you. We normally use just gpl-2.0.
>
I think more than "we", the subsystem dictates licensing. All FWU code
is under GPL-2.0-or-later.


> >>
> >> As I said this DT binding should be approved first to make sure that we don't
> >> need to fix DT binding in future. Just simply do it right from the begining.
> >>
> > Yes, I will cc Rob in the next submission (I only forgot last time).
> > However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> > I am not sure if there is any reason for the fwu node to even be in
> > the dts for kernel. But sure it is good to have it eyeballed by the DT
> > gods.
>
> It doesn't really go to kernel.
> Simon pushed options node directly to dt-schema
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
> And that would be also location for this node too.
>
Yes, but I have resend the already existisng bindings in uboot. My
patch only modified them. Not a big problem.

-j
Ilias Apalodimas May 19, 2023, 12:21 p.m. UTC | #5
HI Jassi, Michal

Based on the discussion we had on the dt bindings, I am personally ok with
the notion of having those defined internally until we can prove it makes
sense for the to be sent to the dt-schema.

In the future we need to strip those from U-Boot, before we hand over the
DR to the OS, but this is a problem that already exists regardless of this patchset.

Jassi, Michal had some review comments.  Are you going to send a v5 with
those fixed?

Thanks
/Ilias

On Fri, Apr 14, 2023 at 10:09:11AM -0500, Jassi Brar wrote:
> On Fri, Apr 14, 2023 at 8:56 AM Michal Simek <michal.simek@amd.com> wrote:
> > On 4/10/23 05:56, Jassi Brar wrote:
> > > On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek@amd.com> wrote:
> > >> On 3/27/23 23:15, jassisinghbrar@gmail.com wrote:
> > >
> > >>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > >>> new file mode 100644
> > >>> index 0000000000..4b1a10073a
> > >>> --- /dev/null
> > >>> +++ b/drivers/fwu-mdata/raw_mtd.c
> > >>> @@ -0,0 +1,272 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0+
> > >>
> > >> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
> > >>
> > > just c&p. though isn't that the same as GPL-2.0-or-later ?
> >
> > license choice is up to you. We normally use just gpl-2.0.
> >
> I think more than "we", the subsystem dictates licensing. All FWU code
> is under GPL-2.0-or-later.
> 
> 
> > >>
> > >> As I said this DT binding should be approved first to make sure that we don't
> > >> need to fix DT binding in future. Just simply do it right from the begining.
> > >>
> > > Yes, I will cc Rob in the next submission (I only forgot last time).
> > > However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> > > I am not sure if there is any reason for the fwu node to even be in
> > > the dts for kernel. But sure it is good to have it eyeballed by the DT
> > > gods.
> >
> > It doesn't really go to kernel.
> > Simon pushed options node directly to dt-schema
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
> > And that would be also location for this node too.
> >
> Yes, but I have resend the already existisng bindings in uboot. My
> patch only modified them. Not a big problem.
> 
> -j
Michal Simek May 19, 2023, 12:45 p.m. UTC | #6
Hi Ilias,

On 5/19/23 14:21, Ilias Apalodimas wrote:
> HI Jassi, Michal
> 
> Based on the discussion we had on the dt bindings, I am personally ok with
> the notion of having those defined internally until we can prove it makes
> sense for the to be sent to the dt-schema.
> 
> In the future we need to strip those from U-Boot, before we hand over the
> DR to the OS, but this is a problem that already exists regardless of this patchset.
> 
> Jassi, Michal had some review comments.  Are you going to send a v5 with
> those fixed?

We created this patch for dt-schema and if there is no comment I will be sending 
it to Rob to get merge.

lore.kernel.org/r/ca0715934133dbce6a5a3fd91483e0af92ea8ac6.1683815597.git.michal.simek@amd.com

As I said I am fine with having it in u-boot tree only but that node has to be 
removed to pass certification.
It means I would expect that we will solve it together not later on because it 
will catch us very quickly.
If you have code in EFI to do that it, let's just do it together to be able to 
pass IR2.0 requirements without waiting for another patch.

Thanks,
Michal
Tom Rini May 19, 2023, 8:54 p.m. UTC | #7
On Fri, May 19, 2023 at 02:45:26PM +0200, Michal Simek wrote:
> Hi Ilias,
> 
> On 5/19/23 14:21, Ilias Apalodimas wrote:
> > HI Jassi, Michal
> > 
> > Based on the discussion we had on the dt bindings, I am personally ok with
> > the notion of having those defined internally until we can prove it makes
> > sense for the to be sent to the dt-schema.
> > 
> > In the future we need to strip those from U-Boot, before we hand over the
> > DR to the OS, but this is a problem that already exists regardless of this patchset.
> > 
> > Jassi, Michal had some review comments.  Are you going to send a v5 with
> > those fixed?
> 
> We created this patch for dt-schema and if there is no comment I will be
> sending it to Rob to get merge.
> 
> lore.kernel.org/r/ca0715934133dbce6a5a3fd91483e0af92ea8ac6.1683815597.git.michal.simek@amd.com
> 
> As I said I am fine with having it in u-boot tree only but that node has to
> be removed to pass certification.
> It means I would expect that we will solve it together not later on because
> it will catch us very quickly.
> If you have code in EFI to do that it, let's just do it together to be able
> to pass IR2.0 requirements without waiting for another patch.

So I think in sum, I think the path today is to see if Rob will accept
that patch and if not, either rework based on feedback or if the
feedback is "this is U-Boot centric atm, not appropriate" we'll go with
the path of removing nodes before the OS gets the tree, which I think is
a generic enough bit of framework that we'll need regardless, for 2.0.
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..4b1a10073a
--- /dev/null
+++ b/drivers/fwu-mdata/raw_mtd.c
@@ -0,0 +1,272 @@ 
+// 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 <linux/errno.h>
+#include <linux/types.h>
+
+/* Internal helper structure to move data around */
+struct fwu_mdata_mtd_priv {
+	struct mtd_info *mtd;
+	char pri_label[50];
+	char sec_label[50];
+	u32 pri_offset;
+	u32 sec_offset;
+};
+
+enum fwu_mtd_op {
+	FWU_MTD_READ,
+	FWU_MTD_WRITE,
+};
+
+extern struct fwu_mtd_image_info fwu_mtd_images[];
+
+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);
+}
+
+static int flash_partition_offset(struct udevice *dev, const char *part_name, fdt_addr_t *offset)
+{
+	ofnode node, parts_node;
+	fdt_addr_t size = 0;
+
+	parts_node = ofnode_by_compatible(dev_ofnode(dev), "fixed-partitions");
+	node = ofnode_by_prop_value(parts_node, "label", part_name, strlen(part_name) + 1);
+	if (!ofnode_valid(node)) {
+		log_err("Warning: Failed to find partition by label <%s>\n", part_name);
+		return -ENOENT;
+	}
+
+	*offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
+
+	return (int)size;
+}
+
+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;
+	const char *label;
+	fdt_addr_t offset;
+	int ret, size;
+	u32 phandle;
+	ofnode bank;
+	int off_img;
+
+	/* 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 -ENODEV;
+	}
+
+	/* Get the offset of primary and seconday mdata */
+	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0, &label);
+	if (ret)
+		return ret;
+	strcpy(mtd_priv->pri_label, label);
+
+	ret = flash_partition_offset(mtd_dev, mtd_priv->pri_label, &offset);
+	if (ret <= 0)
+		return ret;
+	mtd_priv->pri_offset = offset;
+
+	ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 1, &label);
+	if (ret)
+		return ret;
+	strcpy(mtd_priv->sec_label, label);
+
+	ret = flash_partition_offset(mtd_dev, mtd_priv->sec_label, &offset);
+	if (ret <= 0)
+		return ret;
+	mtd_priv->sec_offset = offset;
+
+	off_img = 0;
+
+	ofnode_for_each_subnode(bank, dev_ofnode(dev)) {
+		int bank_num, bank_offset, bank_size;
+		const char *bank_name;
+		ofnode image;
+
+		ofnode_read_u32(bank, "id", &bank_num);
+		bank_name = ofnode_read_string(bank, "label");
+		bank_size = flash_partition_offset(mtd_dev, bank_name, &offset);
+		if (bank_size <= 0)
+			return bank_size;
+		bank_offset = offset;
+		log_debug("Bank%d: %s [0x%x - 0x%x]\n",
+			  bank_num, bank_name, bank_offset, bank_offset + bank_size);
+
+		ofnode_for_each_subnode(image, bank) {
+			int image_num, image_offset, image_size;
+			const char *uuid;
+
+			if (off_img == CONFIG_FWU_NUM_BANKS *
+						CONFIG_FWU_NUM_IMAGES_PER_BANK) {
+				log_err("DT provides images more than configured!\n");
+				break;
+			}
+
+			uuid = ofnode_read_string(image, "uuid");
+			ofnode_read_u32(image, "id", &image_num);
+			ofnode_read_u32(image, "offset", &image_offset);
+			ofnode_read_u32(image, "size", &image_size);
+
+			fwu_mtd_images[off_img].start = bank_offset + image_offset;
+			fwu_mtd_images[off_img].size = image_size;
+			fwu_mtd_images[off_img].bank_num = bank_num;
+			fwu_mtd_images[off_img].image_num = image_num;
+			strcpy(fwu_mtd_images[off_img].uuidbuf, uuid);
+			log_debug("\tImage%d: %s @0x%x\n\n",
+				  image_num, uuid, bank_offset + image_offset);
+			off_img++;
+		}
+	}
+
+	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),
+};