[PATCH V3 1/6] dt-bindings: nvmem: layouts: add U-Boot environment variables layout
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
Signed-off-by: Rafał Miłecki rafal@milecki.pl --- V2: Add "-layout" suffix to compatibles to avoid conflict
.../nvmem/layouts/u-boot,env-layout.yaml | 55 +++++++++++++++++++ .../devicetree/bindings/nvmem/u-boot,env.yaml | 6 ++ 2 files changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml
diff --git a/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml new file mode 100644 index 000000000000..3b4d8f2a44e1 --- /dev/null +++ b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env-layout.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NVMEM layout of U-Boot environment variables + +maintainers: + - Rafał Miłecki rafal@milecki.pl + +description: + U-Boot uses environment variables to store device parameters and + configuration. They may be used for booting process, setup or keeping end user + info. + + Data is stored using U-Boot specific formats (variant specific header and NUL + separated key-value pairs). + +properties: + compatible: + oneOf: + - description: A standalone env data block + const: u-boot,env-layout + - description: Two redundant blocks with active one flagged + const: u-boot,env-redundant-bool-layout + - description: Two redundant blocks with active having higher counter + const: u-boot,env-redundant-count-layout + - description: Broadcom's variant with custom header + const: brcm,env-layout + +additionalProperties: false + +examples: + - | + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + reg = <0x0 0x40000>; + label = "u-boot"; + read-only; + }; + + partition@40000 { + reg = <0x40000 0x10000>; + label = "u-boot-env"; + + nvmem-layout { + compatible = "u-boot,env-layout"; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml index 9c36afc7084b..6c2a3ca5f051 100644 --- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml +++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml @@ -26,9 +26,15 @@ description: |
Variables can be defined as NVMEM device subnodes.
+ Introduction of NVMEM layouts exposed a limitation of this binding design. + Description of NVMEM data content should be separated from NVMEM devices. + Since the introduction of U-Boot env data layout this binding is deprecated. + maintainers: - Rafał Miłecki rafal@milecki.pl
+deprecated: true + properties: compatible: oneOf:
From: Rafał Miłecki rafal@milecki.pl
This is required by layouts that need to read whole NVMEM content. It's especially useful for NVMEM devices without hardcoded layout (like U-Boot environment data block).
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/nvmem/core.c | 13 +++++++++++++ include/linux/nvmem-consumer.h | 1 + 2 files changed, 14 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 4ed54076346d..980123fb4dde 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -2163,6 +2163,19 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem) } EXPORT_SYMBOL_GPL(nvmem_dev_name);
+/** + * nvmem_dev_size() - Get the size of a given nvmem device. + * + * @nvmem: nvmem device. + * + * Return: size of the nvmem device. + */ +size_t nvmem_dev_size(struct nvmem_device *nvmem) +{ + return nvmem->size; +} +EXPORT_SYMBOL_GPL(nvmem_dev_size); + static int __init nvmem_init(void) { int ret; diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h index 2d306fa13b1a..34c0e58dfa26 100644 --- a/include/linux/nvmem-consumer.h +++ b/include/linux/nvmem-consumer.h @@ -81,6 +81,7 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem, struct nvmem_cell_info *info, void *buf);
const char *nvmem_dev_name(struct nvmem_device *nvmem); +size_t nvmem_dev_size(struct nvmem_device *nvmem);
void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries);
From: Rafał Miłecki rafal@milecki.pl
Simplify adding NVMEM cells.
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com --- V3: Split PATCH 3/3 into two
drivers/nvmem/u-boot-env.c | 55 +++++++++++++++----------------------- 1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c index c4ae94af4af7..dd9d0ad22712 100644 --- a/drivers/nvmem/u-boot-env.c +++ b/drivers/nvmem/u-boot-env.c @@ -23,13 +23,10 @@ enum u_boot_env_format {
struct u_boot_env { struct device *dev; + struct nvmem_device *nvmem; enum u_boot_env_format format;
struct mtd_info *mtd; - - /* Cells */ - struct nvmem_cell_info *cells; - int ncells; };
struct u_boot_env_image_single { @@ -94,43 +91,36 @@ static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, i static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf, size_t data_offset, size_t data_len) { + struct nvmem_device *nvmem = priv->nvmem; 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; + for (var = data; var < data + data_len && *var; - var = value + strlen(value) + 1, idx++) { + var = value + strlen(value) + 1) { + struct nvmem_cell_info info = {}; + 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) + info.name = devm_kstrdup(dev, var, GFP_KERNEL); + if (!info.name) return -ENOMEM; - priv->cells[idx].offset = data_offset + value - data; - priv->cells[idx].bytes = strlen(value); - priv->cells[idx].np = of_get_child_by_name(dev->of_node, priv->cells[idx].name); + info.offset = data_offset + value - data; + info.bytes = strlen(value); + info.np = of_get_child_by_name(dev->of_node, info.name); if (!strcmp(var, "ethaddr")) { - priv->cells[idx].raw_len = strlen(value); - priv->cells[idx].bytes = ETH_ALEN; - priv->cells[idx].read_post_process = u_boot_env_read_post_process_ethaddr; + info.raw_len = strlen(value); + info.bytes = ETH_ALEN; + info.read_post_process = u_boot_env_read_post_process_ethaddr; } - }
- if (WARN_ON(idx != priv->ncells)) - priv->ncells = idx; + nvmem_add_one_cell(nvmem, &info); + }
return 0; } @@ -209,7 +199,6 @@ static int u_boot_env_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct u_boot_env *priv; - int err;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -224,17 +213,15 @@ static int u_boot_env_probe(struct platform_device *pdev) 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->mtd->size;
- return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config)); + priv->nvmem = devm_nvmem_register(dev, &config); + if (IS_ERR(priv->nvmem)) + return PTR_ERR(priv->nvmem); + + return u_boot_env_parse(priv); }
static const struct of_device_id u_boot_env_of_match_table[] = {
From: Rafał Miłecki rafal@milecki.pl
Use nvmem_dev_size() and nvmem_device_read() to make this driver less mtd dependent.
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com --- V2: Don't introduce memleak when handling nvmem_device_read() failures V3: Split PATCH 3/3 into two
drivers/nvmem/u-boot-env.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c index dd9d0ad22712..111905189341 100644 --- a/drivers/nvmem/u-boot-env.c +++ b/drivers/nvmem/u-boot-env.c @@ -127,27 +127,34 @@ static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf,
static int u_boot_env_parse(struct u_boot_env *priv) { + struct nvmem_device *nvmem = priv->nvmem; 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; + size_t dev_size; uint32_t crc32; uint32_t calc; - size_t bytes; uint8_t *buf; + int bytes; int err;
- buf = kcalloc(1, priv->mtd->size, GFP_KERNEL); + dev_size = nvmem_dev_size(nvmem); + + buf = kcalloc(1, dev_size, GFP_KERNEL); if (!buf) { err = -ENOMEM; goto err_out; }
- err = mtd_read(priv->mtd, 0, priv->mtd->size, &bytes, buf); - if ((err && !mtd_is_bitflip(err)) || bytes != priv->mtd->size) { - dev_err(dev, "Failed to read from mtd: %d\n", err); + bytes = nvmem_device_read(nvmem, 0, dev_size, buf); + if (bytes < 0) { + err = bytes; + goto err_kfree; + } else if (bytes != dev_size) { + err = -EIO; goto err_kfree; }
@@ -169,8 +176,8 @@ static int u_boot_env_parse(struct u_boot_env *priv) break; } crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset)); - crc32_data_len = priv->mtd->size - crc32_data_offset; - data_len = priv->mtd->size - data_offset; + crc32_data_len = dev_size - crc32_data_offset; + data_len = dev_size - data_offset;
calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L; if (calc != crc32) { @@ -179,7 +186,7 @@ static int u_boot_env_parse(struct u_boot_env *priv) goto err_kfree; }
- buf[priv->mtd->size - 1] = '\0'; + buf[dev_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);
From: Rafał Miłecki rafal@milecki.pl
1. Prefer kzalloc() over kcalloc() See memory-allocation.rst which says: "to be on the safe side it's best to use routines that set memory to zero, like kzalloc()" 2. Drop dev_err() for u_boot_env_add_cells() fail It can fail only on -ENOMEM. We don't want to print error then. 3. Add extra "crc32_addr" variable It makes code reading header's crc32 easier to understand / review.
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com --- V2: New patch with suggestions from Miquel
drivers/nvmem/u-boot-env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c index 111905189341..befbab156cda 100644 --- a/drivers/nvmem/u-boot-env.c +++ b/drivers/nvmem/u-boot-env.c @@ -132,6 +132,7 @@ static int u_boot_env_parse(struct u_boot_env *priv) size_t crc32_data_offset; size_t crc32_data_len; size_t crc32_offset; + __le32 *crc32_addr; size_t data_offset; size_t data_len; size_t dev_size; @@ -143,7 +144,7 @@ static int u_boot_env_parse(struct u_boot_env *priv)
dev_size = nvmem_dev_size(nvmem);
- buf = kcalloc(1, dev_size, GFP_KERNEL); + buf = kzalloc(dev_size, GFP_KERNEL); if (!buf) { err = -ENOMEM; goto err_out; @@ -175,7 +176,8 @@ static int u_boot_env_parse(struct u_boot_env *priv) data_offset = offsetof(struct u_boot_env_image_broadcom, data); break; } - crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset)); + crc32_addr = (__le32 *)(buf + crc32_offset); + crc32 = le32_to_cpu(*crc32_addr); crc32_data_len = dev_size - crc32_data_offset; data_len = dev_size - data_offset;
@@ -188,8 +190,6 @@ static int u_boot_env_parse(struct u_boot_env *priv)
buf[dev_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);
From: Rafał Miłecki rafal@milecki.pl
Move all generic (NVMEM devices independent) code from NVMEM device driver to NVMEM layout driver. Then add a simple NVMEM layout code on top of it.
Thanks to proper layout it's possible to support U-Boot env data stored on any kind of NVMEM device.
For backward compatibility with old DT bindings we need to keep old NVMEM device driver functional. To avoid code duplication export and reuse a parsing function.
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com --- V2: Support new compatibles & use device_get_match_data() helper V3: Use imperative in commit body
IMPORTANT: This is based on top of the: [PATCH v6.8 1/2] nvmem: layouts: refactor .add_cells() callback arguments
MAINTAINERS | 1 + drivers/nvmem/Kconfig | 3 +- drivers/nvmem/layouts/Kconfig | 11 ++ drivers/nvmem/layouts/Makefile | 1 + drivers/nvmem/layouts/u-boot-env.c | 204 +++++++++++++++++++++++++++++ drivers/nvmem/layouts/u-boot-env.h | 15 +++ drivers/nvmem/u-boot-env.c | 158 +--------------------- 7 files changed, 235 insertions(+), 158 deletions(-) create mode 100644 drivers/nvmem/layouts/u-boot-env.c create mode 100644 drivers/nvmem/layouts/u-boot-env.h
diff --git a/MAINTAINERS b/MAINTAINERS index b589218605b4..1f7e6d74cd51 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22282,6 +22282,7 @@ U-BOOT ENVIRONMENT VARIABLES M: Rafał Miłecki rafal@milecki.pl S: Maintained F: Documentation/devicetree/bindings/nvmem/u-boot,env.yaml +F: drivers/nvmem/layouts/u-boot-env.c F: drivers/nvmem/u-boot-env.c
UACCE ACCELERATOR FRAMEWORK diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig index 283134498fbc..d2c384f58028 100644 --- a/drivers/nvmem/Kconfig +++ b/drivers/nvmem/Kconfig @@ -363,8 +363,7 @@ config NVMEM_SUNXI_SID config NVMEM_U_BOOT_ENV tristate "U-Boot environment variables support" depends on OF && MTD - select CRC32 - select GENERIC_NET_UTILS + select NVMEM_LAYOUT_U_BOOT_ENV help U-Boot stores its setup as environment variables. This driver adds support for verifying & exporting such data. It also exposes variables diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig index 9c6e672fc350..5e586dfebe47 100644 --- a/drivers/nvmem/layouts/Kconfig +++ b/drivers/nvmem/layouts/Kconfig @@ -26,6 +26,17 @@ config NVMEM_LAYOUT_ONIE_TLV
If unsure, say N.
+config NVMEM_LAYOUT_U_BOOT_ENV + tristate "U-Boot environment variables layout" + select CRC32 + select GENERIC_NET_UTILS + 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 unsure, say N. + endmenu
endif diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile index 2974bd7d33ed..4940c9db0665 100644 --- a/drivers/nvmem/layouts/Makefile +++ b/drivers/nvmem/layouts/Makefile @@ -5,3 +5,4 @@
obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o +obj-$(CONFIG_NVMEM_LAYOUT_U_BOOT_ENV) += u-boot-env.o diff --git a/drivers/nvmem/layouts/u-boot-env.c b/drivers/nvmem/layouts/u-boot-env.c new file mode 100644 index 000000000000..dcd2ffed503c --- /dev/null +++ b/drivers/nvmem/layouts/u-boot-env.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 - 2023 Rafał Miłecki rafal@milecki.pl + */ + +#include <linux/crc32.h> +#include <linux/etherdevice.h> +#include <linux/export.h> +#include <linux/if_ether.h> +#include <linux/nvmem-consumer.h> +#include <linux/nvmem-provider.h> +#include <linux/of.h> +#include <linux/slab.h> + +#include "u-boot-env.h" + +struct u_boot_env_image_single { + __le32 crc32; + uint8_t data[]; +} __packed; + +struct u_boot_env_image_redundant { + __le32 crc32; + u8 mark; + uint8_t data[]; +} __packed; + +struct u_boot_env_image_broadcom { + __le32 magic; + __le32 len; + __le32 crc32; + DECLARE_FLEX_ARRAY(uint8_t, data); +} __packed; + +static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, int index, + unsigned int offset, void *buf, size_t bytes) +{ + u8 mac[ETH_ALEN]; + + if (bytes != 3 * ETH_ALEN - 1) + return -EINVAL; + + if (!mac_pton(buf, mac)) + return -EINVAL; + + if (index) + eth_addr_add(mac, index); + + ether_addr_copy(buf, mac); + + return 0; +} + +static int u_boot_env_parse_cells(struct device *dev, struct nvmem_device *nvmem, uint8_t *buf, + size_t data_offset, size_t data_len) +{ + char *data = buf + data_offset; + char *var, *value, *eq; + + for (var = data; + var < data + data_len && *var; + var = value + strlen(value) + 1) { + struct nvmem_cell_info info = {}; + + eq = strchr(var, '='); + if (!eq) + break; + *eq = '\0'; + value = eq + 1; + + info.name = devm_kstrdup(dev, var, GFP_KERNEL); + if (!info.name) + return -ENOMEM; + info.offset = data_offset + value - data; + info.bytes = strlen(value); + info.np = of_get_child_by_name(dev->of_node, info.name); + if (!strcmp(var, "ethaddr")) { + info.raw_len = strlen(value); + info.bytes = ETH_ALEN; + info.read_post_process = u_boot_env_read_post_process_ethaddr; + } + + nvmem_add_one_cell(nvmem, &info); + } + + return 0; +} + +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem, + enum u_boot_env_format format) +{ + size_t crc32_data_offset; + size_t crc32_data_len; + size_t crc32_offset; + __le32 *crc32_addr; + size_t data_offset; + size_t data_len; + size_t dev_size; + uint32_t crc32; + uint32_t calc; + uint8_t *buf; + int bytes; + int err; + + dev_size = nvmem_dev_size(nvmem); + + buf = kzalloc(dev_size, GFP_KERNEL); + if (!buf) { + err = -ENOMEM; + goto err_out; + } + + bytes = nvmem_device_read(nvmem, 0, dev_size, buf); + if (bytes < 0) { + err = bytes; + goto err_kfree; + } else if (bytes != dev_size) { + err = -EIO; + goto err_kfree; + } + + switch (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, data); + data_offset = offsetof(struct u_boot_env_image_redundant, data); + break; + case U_BOOT_FORMAT_BROADCOM: + crc32_offset = offsetof(struct u_boot_env_image_broadcom, crc32); + crc32_data_offset = offsetof(struct u_boot_env_image_broadcom, data); + data_offset = offsetof(struct u_boot_env_image_broadcom, data); + break; + } + crc32_addr = (__le32 *)(buf + crc32_offset); + crc32 = le32_to_cpu(*crc32_addr); + crc32_data_len = dev_size - crc32_data_offset; + data_len = dev_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[dev_size - 1] = '\0'; + err = u_boot_env_parse_cells(dev, nvmem, buf, data_offset, data_len); + +err_kfree: + kfree(buf); +err_out: + return err; +} +EXPORT_SYMBOL_GPL(u_boot_env_parse); + +static int u_boot_env_add_cells(struct nvmem_layout *layout) +{ + struct device *dev = &layout->dev; + enum u_boot_env_format format; + + format = (uintptr_t)device_get_match_data(dev); + + return u_boot_env_parse(dev, layout->nvmem, format); +} + +static int u_boot_env_probe(struct nvmem_layout *layout) +{ + layout->add_cells = u_boot_env_add_cells; + + return nvmem_layout_register(layout); +} + +static void u_boot_env_remove(struct nvmem_layout *layout) +{ + nvmem_layout_unregister(layout); +} + +static const struct of_device_id u_boot_env_of_match_table[] = { + { .compatible = "u-boot,env-layout", .data = (void *)U_BOOT_FORMAT_SINGLE, }, + { .compatible = "u-boot,env-redundant-bool-layout", .data = (void *)U_BOOT_FORMAT_REDUNDANT, }, + { .compatible = "u-boot,env-redundant-count-layout", .data = (void *)U_BOOT_FORMAT_REDUNDANT, }, + { .compatible = "brcm,env-layout", .data = (void *)U_BOOT_FORMAT_BROADCOM, }, + {}, +}; + +static struct nvmem_layout_driver u_boot_env_layout = { + .driver = { + .owner = THIS_MODULE, + .name = "u-boot-env-layout", + .of_match_table = u_boot_env_of_match_table, + }, + .probe = u_boot_env_probe, + .remove = u_boot_env_remove, +}; +module_nvmem_layout_driver(u_boot_env_layout); + +MODULE_AUTHOR("Rafał Miłecki"); +MODULE_LICENSE("GPL"); +MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table); diff --git a/drivers/nvmem/layouts/u-boot-env.h b/drivers/nvmem/layouts/u-boot-env.h new file mode 100644 index 000000000000..dd5f280ac047 --- /dev/null +++ b/drivers/nvmem/layouts/u-boot-env.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H +#define _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H + +enum u_boot_env_format { + U_BOOT_FORMAT_SINGLE, + U_BOOT_FORMAT_REDUNDANT, + U_BOOT_FORMAT_BROADCOM, +}; + +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem, + enum u_boot_env_format format); + +#endif /* ifndef _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H */ diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c index befbab156cda..386b2b255c30 100644 --- a/drivers/nvmem/u-boot-env.c +++ b/drivers/nvmem/u-boot-env.c @@ -3,23 +3,15 @@ * Copyright (C) 2022 Rafał Miłecki rafal@milecki.pl */
-#include <linux/crc32.h> -#include <linux/etherdevice.h> -#include <linux/if_ether.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.h> #include <linux/platform_device.h> #include <linux/slab.h>
-enum u_boot_env_format { - U_BOOT_FORMAT_SINGLE, - U_BOOT_FORMAT_REDUNDANT, - U_BOOT_FORMAT_BROADCOM, -}; +#include "layouts/u-boot-env.h"
struct u_boot_env { struct device *dev; @@ -29,24 +21,6 @@ struct u_boot_env { struct mtd_info *mtd; };
-struct u_boot_env_image_single { - __le32 crc32; - uint8_t data[]; -} __packed; - -struct u_boot_env_image_redundant { - __le32 crc32; - u8 mark; - uint8_t data[]; -} __packed; - -struct u_boot_env_image_broadcom { - __le32 magic; - __le32 len; - __le32 crc32; - DECLARE_FLEX_ARRAY(uint8_t, data); -} __packed; - static int u_boot_env_read(void *context, unsigned int offset, void *val, size_t bytes) { @@ -69,134 +43,6 @@ static int u_boot_env_read(void *context, unsigned int offset, void *val, return 0; }
-static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, int index, - unsigned int offset, void *buf, size_t bytes) -{ - u8 mac[ETH_ALEN]; - - if (bytes != 3 * ETH_ALEN - 1) - return -EINVAL; - - if (!mac_pton(buf, mac)) - return -EINVAL; - - if (index) - eth_addr_add(mac, index); - - ether_addr_copy(buf, mac); - - 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 nvmem_device *nvmem = priv->nvmem; - struct device *dev = priv->dev; - char *data = buf + data_offset; - char *var, *value, *eq; - - for (var = data; - var < data + data_len && *var; - var = value + strlen(value) + 1) { - struct nvmem_cell_info info = {}; - - eq = strchr(var, '='); - if (!eq) - break; - *eq = '\0'; - value = eq + 1; - - info.name = devm_kstrdup(dev, var, GFP_KERNEL); - if (!info.name) - return -ENOMEM; - info.offset = data_offset + value - data; - info.bytes = strlen(value); - info.np = of_get_child_by_name(dev->of_node, info.name); - if (!strcmp(var, "ethaddr")) { - info.raw_len = strlen(value); - info.bytes = ETH_ALEN; - info.read_post_process = u_boot_env_read_post_process_ethaddr; - } - - nvmem_add_one_cell(nvmem, &info); - } - - return 0; -} - -static int u_boot_env_parse(struct u_boot_env *priv) -{ - struct nvmem_device *nvmem = priv->nvmem; - struct device *dev = priv->dev; - size_t crc32_data_offset; - size_t crc32_data_len; - size_t crc32_offset; - __le32 *crc32_addr; - size_t data_offset; - size_t data_len; - size_t dev_size; - uint32_t crc32; - uint32_t calc; - uint8_t *buf; - int bytes; - int err; - - dev_size = nvmem_dev_size(nvmem); - - buf = kzalloc(dev_size, GFP_KERNEL); - if (!buf) { - err = -ENOMEM; - goto err_out; - } - - bytes = nvmem_device_read(nvmem, 0, dev_size, buf); - if (bytes < 0) { - err = bytes; - goto err_kfree; - } else if (bytes != dev_size) { - err = -EIO; - 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, data); - data_offset = offsetof(struct u_boot_env_image_redundant, data); - break; - case U_BOOT_FORMAT_BROADCOM: - crc32_offset = offsetof(struct u_boot_env_image_broadcom, crc32); - crc32_data_offset = offsetof(struct u_boot_env_image_broadcom, data); - data_offset = offsetof(struct u_boot_env_image_broadcom, data); - break; - } - crc32_addr = (__le32 *)(buf + crc32_offset); - crc32 = le32_to_cpu(*crc32_addr); - crc32_data_len = dev_size - crc32_data_offset; - data_len = dev_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[dev_size - 1] = '\0'; - err = u_boot_env_add_cells(priv, buf, data_offset, data_len); - -err_kfree: - kfree(buf); -err_out: - return err; -} - static int u_boot_env_probe(struct platform_device *pdev) { struct nvmem_config config = { @@ -228,7 +74,7 @@ static int u_boot_env_probe(struct platform_device *pdev) if (IS_ERR(priv->nvmem)) return PTR_ERR(priv->nvmem);
- return u_boot_env_parse(priv); + return u_boot_env_parse(dev, priv->nvmem, priv->format); }
static const struct of_device_id u_boot_env_of_match_table[] = {
On Thu, Dec 21, 2023 at 06:34:21PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
Move all generic (NVMEM devices independent) code from NVMEM device driver to NVMEM layout driver. Then add a simple NVMEM layout code on top of it.
Thanks to proper layout it's possible to support U-Boot env data stored on any kind of NVMEM device.
For backward compatibility with old DT bindings we need to keep old NVMEM device driver functional. To avoid code duplication export and reuse a parsing function.
Signed-off-by: Rafał Miłecki rafal@milecki.pl Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
V2: Support new compatibles & use device_get_match_data() helper V3: Use imperative in commit body
IMPORTANT: This is based on top of the: [PATCH v6.8 1/2] nvmem: layouts: refactor .add_cells() callback arguments
I applied patches 2-5 in this series, I'll let you rebase the remaining two after addressing the issues discussed in patch 1.
thanks,
gre gk-h
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already.
Rob
Hello,
robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already.
Actually I think what Rafał wants to do goes in the right direction but I also understand from a binding perspective it may be a little confusing, even more if we consider "NVMEM" a Linux specific concept.
There is today a "u-boot env" NVMEM *device* description which almost sits at the same level as eg. an eeprom device. We cannot compare "an eeprom device" and "a u-boot environment" of course. But that's truly what is currently described.
* Current situation
Flash device -> U-Boot env layout -> NVMEM cells
* Improved situation
Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
The latter is of course the most relevant description as we expect storage devices to expose a storage-agnostic interface (NVMEM in this case) which can then be parsed (by NVMEM layouts) in a storage agnostic way.
In the current case, the current U-Boot env binding tells people to declare the env layout on top of a flash device (only). The current description also expects a partition node which is typical to flash devices. Whereas what we should have described in the first place is a layout that applies on any kind of NVMEM device.
Bonus point: We've been working the last couple years on clarifying bindings, especially with mtd partitions (with the partitions{} container) and NVMEM layouts (with the nvmem-layout{} container). The switch proposed in this patch makes use of the latter, of course.
Thanks, Miquèl
On 4.01.2024 08:58, Miquel Raynal wrote:
robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already.
Actually I think what Rafał wants to do goes in the right direction but I also understand from a binding perspective it may be a little confusing, even more if we consider "NVMEM" a Linux specific concept.
There is today a "u-boot env" NVMEM *device* description which almost sits at the same level as eg. an eeprom device. We cannot compare "an eeprom device" and "a u-boot environment" of course. But that's truly what is currently described.
Current situation
Flash device -> U-Boot env layout -> NVMEM cells
Improved situation
Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
The latter is of course the most relevant description as we expect storage devices to expose a storage-agnostic interface (NVMEM in this case) which can then be parsed (by NVMEM layouts) in a storage agnostic way.
In the current case, the current U-Boot env binding tells people to declare the env layout on top of a flash device (only). The current description also expects a partition node which is typical to flash devices. Whereas what we should have described in the first place is a layout that applies on any kind of NVMEM device.
Bonus point: We've been working the last couple years on clarifying bindings, especially with mtd partitions (with the partitions{} container) and NVMEM layouts (with the nvmem-layout{} container). The switch proposed in this patch makes use of the latter, of course.
Thanks Miquèl for filling bits I missed in commit description. Despite years in Linux/DT I still struggle with more complex designs documentation.
As per Rob's comment I think I see his point and a possible design confusion. If you look from a pure DT perspective then "partitions" and "nvmem-layout" serve a very similar purpose. They describe device's data content structure. For fixed structures we have very similar "fixed-partitions" and "fixed-cells".
If we were to design those bindings today I'm wondering if we couldn't have s/partitions/layout/ and s/nvmem-layout/layout/.
Rob: other than having different bindings for MTD vs. NVMEM layouts I think they overall design makes sense. A single device may have content structurized on more than 1 level: 1. You may have fixed layout at top level (multiple partitions) 2. Single partitions may have their own layouts (like U-Boot env data)
Maybe ideally above should look more like:
flash@0 { compatible = "<flash-compatible>";
layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>;
partition@0 { reg = <0x0 0x40000>; label = "u-boot"; };
partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env";
layout { compatible = "u-boot,env-layout"; }; };
partition@50000 { reg = <0x50000 0x100000>; label = "u-boot"; }; }; };
but I can clearly see a use for nested "layout"s. As I said maybe we just shouldn't be so open in calling those MTD or NVMEM devices as that is kind of Linux specific.
I'm not sure if we should try renaming "nvmem-layout" to "layout" or "partitions" in similar way at this point.
On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote:
On 4.01.2024 08:58, Miquel Raynal wrote:
robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already.
Actually I think what Rafał wants to do goes in the right direction but I also understand from a binding perspective it may be a little confusing, even more if we consider "NVMEM" a Linux specific concept.
There is today a "u-boot env" NVMEM *device* description which almost sits at the same level as eg. an eeprom device. We cannot compare "an eeprom device" and "a u-boot environment" of course. But that's truly what is currently described.
Current situation
Flash device -> U-Boot env layout -> NVMEM cells
Isn't it?:
Flash device -> fixed-partitions -> U-Boot env layout -> NVMEM cells
Improved situation
Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
Why is this better? We don't need a container to say 'this is NVMEM stuff' or 'this is MTD stuff'. 'U-Boot env layout' can tell us 'this is NVMEM stuff' or whatever the kernel decides in the future.
The latter is of course the most relevant description as we expect storage devices to expose a storage-agnostic interface (NVMEM in this case) which can then be parsed (by NVMEM layouts) in a storage agnostic way.
In the current case, the current U-Boot env binding tells people to declare the env layout on top of a flash device (only). The current description also expects a partition node which is typical to flash devices. Whereas what we should have described in the first place is a layout that applies on any kind of NVMEM device.
Bonus point: We've been working the last couple years on clarifying bindings, especially with mtd partitions (with the partitions{} container) and NVMEM layouts (with the nvmem-layout{} container). The switch proposed in this patch makes use of the latter, of course.
Thanks Miquèl for filling bits I missed in commit description. Despite years in Linux/DT I still struggle with more complex designs documentation.
As per Rob's comment I think I see his point and a possible design confusion. If you look from a pure DT perspective then "partitions" and "nvmem-layout" serve a very similar purpose. They describe device's data content structure. For fixed structures we have very similar "fixed-partitions" and "fixed-cells".
If we were to design those bindings today I'm wondering if we couldn't have s/partitions/layout/ and s/nvmem-layout/layout/.
Why!? It is just a name, and we can't get rid of the old names. We don't need 2 names.
Rob: other than having different bindings for MTD vs. NVMEM layouts I think they overall design makes sense. A single device may have content structurized on more than 1 level:
- You may have fixed layout at top level (multiple partitions)
- Single partitions may have their own layouts (like U-Boot env data)
Sure. Partitions is for 1 and Layouts is for 2.
Maybe ideally above should look more like:
flash@0 { compatible = "<flash-compatible>";
layout { compatible = "fixed-layout";
Why does 'partitions' and 'fixed-partitions' not work here?
#address-cells = <1>; #size-cells = <1>; partition@0 { reg = <0x0 0x40000>; label = "u-boot"; }; partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; layout { compatible = "u-boot,env-layout"; }; }; partition@50000 { reg = <0x50000 0x100000>; label = "u-boot"; };}; };
but I can clearly see a use for nested "layout"s. As I said maybe we just shouldn't be so open in calling those MTD or NVMEM devices as that is kind of Linux specific.
The overall structure should be agnostic to the subsystem. Specific compatibles like 'u-boot,env' can be tied to a subsystem.
Maybe some things need to be both MTD and NVMEM. MTD to operate on the opague region and NVMEM to access the contents.
I'm not sure if we should try renaming "nvmem-layout" to "layout" or "partitions" in similar way at this point.
You can't rename. It's an ABI though maybe the whole "nvmem-layout" is new enough we can. It's looking like it was a mistake to accept any of this.
Rob
Hi Rob,
robh@kernel.org wrote on Mon, 15 Jan 2024 11:09:03 -0600:
On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote:
On 4.01.2024 08:58, Miquel Raynal wrote:
robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki rafal@milecki.pl
U-Boot env data is a way of storing firmware variables. It's a format that can be used of top of various storage devices. Its binding should be an NVMEM layout instead of a standalone device.
This patch adds layout binding which allows using it on top of MTD NVMEM device as well as any other. At the same time it deprecates the old combined binding.
I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already.
Actually I think what Rafał wants to do goes in the right direction but I also understand from a binding perspective it may be a little confusing, even more if we consider "NVMEM" a Linux specific concept.
There is today a "u-boot env" NVMEM *device* description which almost sits at the same level as eg. an eeprom device. We cannot compare "an eeprom device" and "a u-boot environment" of course. But that's truly what is currently described.
Current situation
Flash device -> U-Boot env layout -> NVMEM cells
Isn't it?:
Flash device -> fixed-partitions -> U-Boot env layout -> NVMEM cells
Improved situation
Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
Why is this better? We don't need a container to say 'this is NVMEM stuff' or 'this is MTD stuff'. 'U-Boot env layout' can tell us 'this is NVMEM stuff' or whatever the kernel decides in the future.
Yes, I also want the U-boot env layout to tell us "this is nvmem stuff". But that's not the case today. Today, it says "this is NVMEM stuff on top of mtd stuff". This was a mistake in the first place, but this compatible is heavily tight to mtd and cannot work on anything else. And correcting this is IMO the right direction.
The latter is of course the most relevant description as we expect storage devices to expose a storage-agnostic interface (NVMEM in this case) which can then be parsed (by NVMEM layouts) in a storage agnostic way.
In the current case, the current U-Boot env binding tells people to declare the env layout on top of a flash device (only). The current description also expects a partition node which is typical to flash devices. Whereas what we should have described in the first place is a layout that applies on any kind of NVMEM device.
Bonus point: We've been working the last couple years on clarifying bindings, especially with mtd partitions (with the partitions{} container) and NVMEM layouts (with the nvmem-layout{} container). The switch proposed in this patch makes use of the latter, of course.
Thanks Miquèl for filling bits I missed in commit description. Despite years in Linux/DT I still struggle with more complex designs documentation.
As per Rob's comment I think I see his point and a possible design confusion. If you look from a pure DT perspective then "partitions" and "nvmem-layout" serve a very similar purpose. They describe device's data content structure. For fixed structures we have very similar "fixed-partitions" and "fixed-cells".
If we were to design those bindings today I'm wondering if we couldn't have s/partitions/layout/ and s/nvmem-layout/layout/.
Why!? It is just a name, and we can't get rid of the old names. We don't need 2 names.
We need 2 names because we are not capturing the same concepts here?
Rob: other than having different bindings for MTD vs. NVMEM layouts I think they overall design makes sense. A single device may have content structurized on more than 1 level:
- You may have fixed layout at top level (multiple partitions)
- Single partitions may have their own layouts (like U-Boot env data)
Sure. Partitions is for 1 and Layouts is for 2.
Maybe ideally above should look more like:
flash@0 { compatible = "<flash-compatible>";
layout { compatible = "fixed-layout";
Why does 'partitions' and 'fixed-partitions' not work here?
They do, and that's actually what we use. This example just illustrates another proposal from Rafal. No panic :)
#address-cells = <1>; #size-cells = <1>; partition@0 { reg = <0x0 0x40000>; label = "u-boot"; }; partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; layout { compatible = "u-boot,env-layout"; }; }; partition@50000 { reg = <0x50000 0x100000>; label = "u-boot"; };}; };
but I can clearly see a use for nested "layout"s. As I said maybe we just shouldn't be so open in calling those MTD or NVMEM devices as that is kind of Linux specific.
The overall structure should be agnostic to the subsystem. Specific compatibles like 'u-boot,env' can be tied to a subsystem.
Maybe some things need to be both MTD and NVMEM. MTD to operate on the opague region and NVMEM to access the contents.
I'm not sure if we should try renaming "nvmem-layout" to "layout" or "partitions" in similar way at this point.
You can't rename. It's an ABI though maybe the whole "nvmem-layout" is new enough we can. It's looking like it was a mistake to accept any of this.
I don't think so.
A partition and a layout are not the same concept, as acknowledged above. We need both, and we need both because we can encapsulate both as well:
flash { partitions { partA@x { layout { cell@Y } } } }
Renaming nvmem-layout to layout can be done if you want, I don't mind, but I don't see the point in doing that.
Thanks, Miquèl
participants (4)
-
Greg Kroah-Hartman -
Miquel Raynal -
Rafał Miłecki -
Rob Herring