From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 51098C636CD for ; Sun, 5 Feb 2023 04:10:08 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0AF7E8580A; Sun, 5 Feb 2023 05:10:05 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="QnwXkGB2"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 92F9E85811; Sun, 5 Feb 2023 05:10:01 +0100 (CET) Received: from mail-oa1-x36.google.com (mail-oa1-x36.google.com [IPv6:2001:4860:4864:20::36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 9025385764 for ; Sun, 5 Feb 2023 05:09:57 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=jaswinder.singh@linaro.org Received: by mail-oa1-x36.google.com with SMTP id 586e51a60fabf-1685cf2003aso11442000fac.12 for ; Sat, 04 Feb 2023 20:09:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OVerMqoL/16caI/yzqf8/UpDuE0WeLoswQIcAkitrfE=; b=QnwXkGB2MIWSpmIAVpUt0v8rYTE9/eXQY5XEb1Qf11Cvp93l+ncmX2q8Yiul6T/iIU kN+aOTP4rGTb9/HjtgQ/PZQ6Rnh7syiGO0zI6sFWL8y5hS7GeKwmCfM/+Acpd4Fc21+z CTSkRX6BHl+fBAD8Ih38Z9EyY+QheTQho3cdY18NzVxGabsmVyCvl/2/aOZv7+brdnlo ISKuwwLKEGVB3Wg1Y+M7h/mEsJpdPJLPuodhzWwWOWVGSoPfGuCj82ggxoQHAeAcqamj jZ20YkmQOSh8Ef6AYBaQ7qEYMgV7jzoNarWfyuBnIv6t0hbyaviHWA1VECHYy7FUwb8j 23ZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OVerMqoL/16caI/yzqf8/UpDuE0WeLoswQIcAkitrfE=; b=rKpJts63aUAbGbsF1e3BnGVXE70Gh9KZNWzd7UevnxMMCWHZNis6Ny77COjW6IXPQq mnQrfAapz9ZL1D84N7LJ8Zv7F2b6jv8DX/VumY1pcuMRXLsJxDUNUc//wwao2fJkBYYG 2DDQj4/eHoy1JLrvFaLjf/iC4ETiBFlpPLu4PBY3u5ojVtBFcpC7pC9xOx6OqgF9f4j3 8FimJmvYBmQDnQ8uu3c0tl+Ne0w+M+XGvvBjGmBjRTpRhJxCznKfW3hG/qOrAsquz8b8 K2/is8xbvR/k23WiStR9sNhFgSOzfwxHLMT6x5dvlSXFFeLGhMu+89Zcf+dymu7Y/MTc lEVg== X-Gm-Message-State: AO0yUKVSn3pO3hKTk5cd4KcyMY9V1jbsq4/bQ/2cVaqd4gnvzVIqQKn3 5+TO5L3ihhQulqZw+IYm5uVT97cxe2A9fnqaWC7w0Q== X-Google-Smtp-Source: AK7set/YOQVOs9OP9N4S5s+adbHJMIZwv9xwrkQXivLF0c2ReuIP3t1qw4BO/gy4EhDQ0Idzf44Uy4NZgEeXgvV9F2g= X-Received: by 2002:a05:6870:2199:b0:163:b0c5:f919 with SMTP id l25-20020a056870219900b00163b0c5f919mr896623oae.14.1675570195901; Sat, 04 Feb 2023 20:09:55 -0800 (PST) MIME-Version: 1.0 References: <20230109010601.578439-1-jaswinder.singh@linaro.org> <20230109010642.578484-1-jaswinder.singh@linaro.org> In-Reply-To: From: Jassi Brar Date: Sat, 4 Feb 2023 22:09:45 -0600 Message-ID: Subject: Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions To: Michal Simek Cc: Jassi Brar , u-boot@lists.denx.de, ilias.apalodimas@linaro.org, etienne.carriere@linaro.org, trini@konsulko.com, sjg@chromium.org, sughosh.ganu@linaro.org, xypron.glpk@gmx.de, takahiro.akashi@linaro.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Wed, 18 Jan 2023 at 08:24, Michal Simek wrote: > > > > On 1/9/23 02:06, Jassi Brar wrote: > > From: Sughosh Ganu > > > > 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 > > Signed-off-by: Jassi Brar > > --- > > 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 > > +#include > > +#include > > +#include > > sort them. > actually flash.h is not required. > > + > > +#include > > +#include > > + > > +/* 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