From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+
+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 <[email protected]>
+deprecated: true
+
properties:
compatible:
oneOf:
--
2.35.3
From: Rafał Miłecki <[email protected]>
1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
mtd dependent
2. Use nvmem_add_one_cell() to simplify adding NVMEM cells
Signed-off-by: Rafał Miłecki <[email protected]>
Reviewed-by: Miquel Raynal <[email protected]>
---
V2: Don't introduce memleak when handling nvmem_device_read() failures
drivers/nvmem/u-boot-env.c | 78 ++++++++++++++++++--------------------
1 file changed, 36 insertions(+), 42 deletions(-)
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index c4ae94af4af7..111905189341 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,70 +91,70 @@ 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;
}
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;
}
@@ -179,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) {
@@ -189,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);
@@ -209,7 +206,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 +220,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[] = {
--
2.35.3
On Tue, Dec 19, 2023 at 06:40:23PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
> mtd dependent
> 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells
Shouldn't this be 2 different patches?
thanks,
greg k-h
On 19.12.2023 19:13, Greg Kroah-Hartman wrote:
> On Tue, Dec 19, 2023 at 06:40:23PM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
>> mtd dependent
>> 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells
>
> Shouldn't this be 2 different patches?
I used to maintainers complaining my patches are too small and not the
other way ;) I think it happened two or three times with mtd subsys.
Sure, I can split it.
Hi Rafał,
[email protected] wrote on Tue, 19 Dec 2023 19:16:37 +0100:
> On 19.12.2023 19:13, Greg Kroah-Hartman wrote:
> > On Tue, Dec 19, 2023 at 06:40:23PM +0100, Rafał Miłecki wrote:
> >> From: Rafał Miłecki <[email protected]>
> >>
> >> 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
> >> mtd dependent
> >> 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells
> >
> > Shouldn't this be 2 different patches?
>
> I used to maintainers complaining my patches are too small and not the
> other way ;) I think it happened two or three times with mtd subsys.
A single patch may be too small if it's alone and we don't see the big
picture, otherwise I have no issue with small patches, do I? Anyway, in
this case I don't mind the patch being split or kept like this, just
keep my R-by applied to both if you do indeed split.
Thanks,
Miquèl