Hello Rafał, On 11.06.22 22:46, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@xxxxxxxxxx> > > U-Boot stores its setup as environment variables. It's a list of > key-value pairs stored on flash device with a custom header. > > This commit adds an NVMEM driver that: > 1. Provides NVMEM access to environment vars binary data > 2. Extracts variables as NVMEM cells > > Current Linux's NVMEM sysfs API allows reading whole NVMEM data block. > It can be used by user-space tools for reading U-Boot env vars block > without the hassle of finding its location. Parsing will still need to > be re-done there. > > Kernel-parsed NVMEM cells can be read however by Linux drivers. This may > be uesful for Ethernet drivers for reading device MAC address which is useful. > often stored as U-Boot env variable. > > Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx> > --- > V3: Use of_get_mtd_device_by_node() (thanks Ahmad) & update description Looks better now! > V2: Drop ARCH_BCM4908 dependency as there are plenty architectures using > U-Boot bootloader. Thanks Srinivas. > > As noticed by Ahmad a missing NVMEM subsystem feature is user-space > access to parsed NVMEM cells. That is something I started working on > some time ago and I'm planning to get back to at some point, please > check: > [PATCH 2/2] nvmem: expose NVMEM cells in sysfs > https://lore.kernel.org/lkml/20211220064730.28806-2-zajec5@xxxxxxxxx/ Thanks for the link. > --- > MAINTAINERS | 1 + > drivers/nvmem/Kconfig | 11 ++ > drivers/nvmem/Makefile | 2 + > drivers/nvmem/u-boot-env.c | 231 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 245 insertions(+) > create mode 100644 drivers/nvmem/u-boot-env.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 475e28365385..43b427fa76b0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20411,6 +20411,7 @@ U-BOOT ENVIRONMENT VARIABLES > M: Rafał Miłecki <rafal@xxxxxxxxxx> > S: Maintained > F: Documentation/devicetree/bindings/nvmem/u-boot,env.yaml > +F: drivers/nvmem/u-boot-env.c > > UACCE ACCELERATOR FRAMEWORK > M: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig > index d72d879a6d34..5f1b32b953b9 100644 > --- a/drivers/nvmem/Kconfig > +++ b/drivers/nvmem/Kconfig > @@ -344,4 +344,15 @@ config NVMEM_APPLE_EFUSES > This driver can also be built as a module. If so, the module will > be called nvmem-apple-efuses. > > +config NVMEM_U_BOOT_ENV > + tristate "U-Boot environment variables support" > + depends on OF && MTD > + select CRC32 > + help > + U-Boot stores its setup as environment variables. This driver adds > + support for verifying & exporting such data. It also exposes variables > + as NVMEM cells so they can be referenced by other drivers. > + > + If compiled as module it will be called nvmem_u-boot-env. You should probably mention that this is tied to MTD somewhere in the help text as u-boot env on EEPROM/raw eMMC is not covered by this. > + > endif > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile > index c710b64f9fe4..399f9972d45b 100644 > --- a/drivers/nvmem/Makefile > +++ b/drivers/nvmem/Makefile > @@ -69,3 +69,5 @@ obj-$(CONFIG_NVMEM_APPLE_EFUSES) += nvmem-apple-efuses.o > nvmem-apple-efuses-y := apple-efuses.o > obj-$(CONFIG_MICROCHIP_OTPC) += nvmem-microchip-otpc.o > nvmem-microchip-otpc-y := microchip-otpc.o > +obj-$(CONFIG_NVMEM_U_BOOT_ENV) += nvmem_u-boot-env.o > +nvmem_u-boot-env-y := u-boot-env.o > diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c > new file mode 100644 > index 000000000000..92c2dd11d99f > --- /dev/null > +++ b/drivers/nvmem/u-boot-env.c > @@ -0,0 +1,231 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Rafał Miłecki <rafal@xxxxxxxxxx> > + */ > + > +#include <linux/crc32.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mtd/mtd.h> > +#include <linux/nvmem-consumer.h> > +#include <linux/nvmem-provider.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > + > +enum u_boot_env_format { > + U_BOOT_FORMAT_SINGLE, > + U_BOOT_FORMAT_REDUNDANT, > +}; > + > +struct u_boot_env { > + struct device *dev; > + enum u_boot_env_format format; > + > + /* Parent device */ > + struct mtd_info *mtd; > + size_t offset; > + size_t size; > + > + /* Cells */ > + struct nvmem_cell_info *cells; > + int ncells; > +}; > + > +struct u_boot_env_image_single { > + __le32 crc32; > + uint8_t data[0]; GCC zero-length arrays are being phased out in favor of flexible array members. Just replace the [0] with []. See Documentation/process/deprecated.rst for more information. > +} __packed; > + > +struct u_boot_env_image_redundant { > + __le32 crc32; > + u8 mark; > + uint8_t data[0]; Same here > +} __packed; > + > +static int u_boot_env_read(void *context, unsigned int offset, void *val, > + size_t bytes) > +{ > + struct u_boot_env *priv = context; > + struct device *dev = priv->dev; > + size_t bytes_read; > + int err; > + > + err = mtd_read(priv->mtd, priv->offset + offset, bytes, &bytes_read, val); Nitpick: if you called u_boot_env::offset u_boot_env::start instead, code would be more readable IMO. > + if (err && !mtd_is_bitflip(err)) { > + dev_err(dev, "Failed to read from mtd: %d\n", err); Nitpick: %pe > + return err; > + } > + > + if (bytes_read != bytes) { > + dev_err(dev, "Failed to read %zd bytes\n", bytes); %zu > + return -EIO; > + } > + > + return 0; > +} > + > +static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf, > + size_t data_offset, size_t data_len) > +{ > + struct device *dev = priv->dev; > + char *data = buf + data_offset; > + char *var, *value, *eq; > + int idx; > + > + priv->ncells = 0; > + for (var = data; var < data + data_len && *var; var += strlen(var) + 1) > + priv->ncells++; > + > + priv->cells = devm_kcalloc(dev, priv->ncells, sizeof(*priv->cells), GFP_KERNEL); > + if (!priv->cells) > + return -ENOMEM; > + > + for (var = data, idx = 0; > + var < data + data_len && *var; > + var = value + strlen(value) + 1, idx++) { > + eq = strchr(var, '='); > + if (!eq) > + break; > + *eq = '\0'; > + value = eq + 1; > + > + priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL); > + if (!priv->cells[idx].name) > + return -ENOMEM; > + priv->cells[idx].offset = data_offset + value - data; > + priv->cells[idx].bytes = strlen(value); U-Boot environment can't hold binary values? > + } > + > + if (WARN_ON(idx != priv->ncells)) > + priv->ncells = idx; > + > + return 0; > +} > + > +static int u_boot_env_parse(struct u_boot_env *priv) > +{ > + struct device *dev = priv->dev; > + size_t crc32_data_offset; > + size_t crc32_data_len; > + size_t crc32_offset; > + size_t data_offset; > + size_t data_len; > + uint32_t crc32; > + uint32_t calc; > + size_t bytes; > + uint8_t *buf; > + int err; > + > + buf = kcalloc(1, priv->size, GFP_KERNEL); > + if (!buf) { > + err = -ENOMEM; > + goto err_out; > + } > + > + err = mtd_read(priv->mtd, priv->offset, priv->size, &bytes, buf); > + if ((err && !mtd_is_bitflip(err)) || bytes != priv->size) { > + dev_err(dev, "Failed to read from mtd: %d\n", err); > + goto err_kfree; > + } > + > + switch (priv->format) { > + case U_BOOT_FORMAT_SINGLE: > + crc32_offset = offsetof(struct u_boot_env_image_single, crc32); > + crc32_data_offset = offsetof(struct u_boot_env_image_single, data); > + data_offset = offsetof(struct u_boot_env_image_single, data); > + break; > + case U_BOOT_FORMAT_REDUNDANT: > + crc32_offset = offsetof(struct u_boot_env_image_redundant, crc32); > + crc32_data_offset = offsetof(struct u_boot_env_image_redundant, mark); > + data_offset = offsetof(struct u_boot_env_image_redundant, data); > + break; > + } > + crc32 = le32_to_cpu(*(uint32_t *)(buf + crc32_offset)); > + crc32_data_len = priv->size - crc32_data_offset; > + data_len = priv->size - data_offset; > + > + calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L; > + if (calc != crc32) { > + dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32); > + err = -EINVAL; > + goto err_kfree; > + } > + > + buf[priv->size - 1] = '\0'; > + err = u_boot_env_add_cells(priv, buf, data_offset, data_len); > + if (err) > + dev_err(dev, "Failed to add cells: %d\n", err); > + > +err_kfree: > + kfree(buf); > +err_out: > + return err; > +} > + > +static const struct of_device_id u_boot_env_of_match_table[] = { > + { .compatible = "u-boot,env", .data = (void *)U_BOOT_FORMAT_SINGLE, }, > + { .compatible = "u-boot,env-redundant-bool", .data = (void *)U_BOOT_FORMAT_REDUNDANT, }, > + { .compatible = "u-boot,env-redundant-count", .data = (void *)U_BOOT_FORMAT_REDUNDANT, }, > + {}, > +}; > + > +static int u_boot_env_probe(struct platform_device *pdev) > +{ > + struct nvmem_config config = { > + .name = "u-boot-env", > + .reg_read = u_boot_env_read, > + }; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + const struct of_device_id *of_id; > + struct u_boot_env *priv; > + int err; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + priv->dev = dev; > + > + of_id = of_match_device(u_boot_env_of_match_table, dev); > + if (!of_id) > + return -EINVAL; No need for the check. > + priv->format = (uintptr_t)of_id->data; of_device_get_match_data() can be used instead and compatible list moved to the bottom. > + > + if (of_property_read_u32(np, "reg", (u32 *)&priv->offset) || bad idea. Not 64-bit safe. > + of_property_read_u32_index(np, "reg", 1, (u32 *)&priv->size)) { Likewise. Also you don't take #address-cells, #size-cells into account. There's of_translate_address() for that, not sure if there's something more specific. But parsing reg like this is not the right way. > + dev_err(dev, "Failed to read \"reg\" property\n"); > + return -EINVAL; > + } > + > + priv->mtd = of_get_mtd_device_by_node(np->parent); > + if (IS_ERR(priv->mtd)) { > + dev_err(dev, "Failed to get %pOF MTD: %ld\n", np->parent, PTR_ERR(priv->mtd)); dev_err -> dev_err_probe. To take care of EPROBE_DEFER. > + return PTR_ERR(priv->mtd); > + } > + > + err = u_boot_env_parse(priv); > + if (err) > + return err; > + > + config.dev = dev; > + config.cells = priv->cells; > + config.ncells = priv->ncells; > + config.priv = priv; > + config.size = priv->size; > + > + return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config)); > +} > + > +static struct platform_driver u_boot_env_driver = { > + .probe = u_boot_env_probe, > + .driver = { > + .name = "u_boot_env", > + .of_match_table = u_boot_env_of_match_table, > + }, > +}; > +module_platform_driver(u_boot_env_driver); > + > +MODULE_AUTHOR("Rafał Miłecki"); > +MODULE_LICENSE("GPL"); > +MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table); Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
