[PATCH 0/5] environment: upstream-NVMEM-compliant OF binding

[PATCH 0/5] environment: upstream-NVMEM-compliant OF binding

[Ahmad Fatoum]

While we have had our binding for a long time, it was never submitted
for upstream inclusion in the Linux DT bindings and it's unlikely to be
accepted as there are no other bindings that reference a fixed partition
in a property.

Linux has support for a u-boot,env binding, which is used by giving the
partition a compatible = "u-boot,env" property.

This series supports the same thing for barebox and allows barebox,environment
to be a subnode of a fixed-partitions node. We intentionally don't add
file-path support for now as the intention is to flesh out this binding
together with upstream.

Ahmad Fatoum (5):
  of: partition: don't parse nvmem-cells in legacy OF binding
  nvmem: probe nvmem-cells container via driver model
  environment: register barebox env OF driver regardless of /chosen
  environment: add support for a single node barebox,environment binding
  sandbox: switch to new barebox environment binding

 .../bindings/nvmem/barebox,environment.yaml   | 55 +++++++++++++
 arch/sandbox/dts/sandbox.dts                  |  8 +-
 drivers/nvmem/partition.c                     | 23 +++++-
 drivers/of/barebox.c                          | 77 +++++++++----------
 drivers/of/partition.c                        | 13 +---
 include/linux/nvmem-provider.h                |  6 --
 6 files changed, 118 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/barebox,environment.yaml

-- 
2.39.5

[PATCH 1/5] of: partition: don't parse nvmem-cells in legacy OF binding

[Ahmad Fatoum]

There are not many upstream instances of nvmem-cells, but all
of them place nvmem-cells inside a partitions container and the
nvmem-cells binding was added way later than the fixed-partitions
binding.

Therefore, let's simplify the code by supporting nvmem-cells only
inside a fixed-partitions container.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/partition.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index df66751fe94d..70297da9d231 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -117,8 +117,6 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
  *      Unfortunately, there is no completely reliable way
  *      to differentiate partitions from devices prior to
  *      probing, because partitions may also have compatibles.
- *      We only handle nvmem-cells, so anything besides that
- *      is assumed to be a device that should be probed directly.
  *
  * Returns zero on success or a negative error code otherwise
  */
@@ -146,8 +144,7 @@ int of_partition_ensure_probed(struct device_node *np)
 	 }
 
 	/* Check if legacy partitions binding */
-	if (!of_property_present(np, "compatible") ||
-	    of_device_is_compatible(np, "nvmem-cells"))
+	if (!of_property_present(np, "compatible"))
 		return of_device_ensure_probed(parent);
 
 	/* Doesn't look like a partition, so let's probe directly */
-- 
2.39.5

[PATCH 2/5] nvmem: probe nvmem-cells container via driver model

[Ahmad Fatoum]

Instead of adding NVMEM-related code into the generic OF partition
parsing, let's instead instantiate a device and bind to it via the
driver model.

This is also what Linux is currently doing for the u-boot,env driver
when not using the layout binding. The nvmem-cells binding itself is
using a MTD notifier, but for our purposes just using the driver model
is ok.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/nvmem/partition.c      | 23 +++++++++++++++++++++--
 drivers/of/partition.c         |  8 ++------
 include/linux/nvmem-provider.h |  6 ------
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/nvmem/partition.c b/drivers/nvmem/partition.c
index 14907e05ba2d..d1127c1401af 100644
--- a/drivers/nvmem/partition.c
+++ b/drivers/nvmem/partition.c
@@ -18,9 +18,15 @@ static int nvmem_cdev_read(void *ctx, unsigned offset, void *buf, size_t bytes)
 	return cdev_read(ctx, buf, bytes, offset, 0);
 }
 
-struct nvmem_device *nvmem_partition_register(struct cdev *cdev)
+static int nvmem_cells_probe(struct device *dev)
 {
 	struct nvmem_config config = {};
+	struct device_node *node = dev->of_node;
+	struct cdev *cdev;
+
+	cdev = cdev_by_device_node(node);
+	if (!cdev)
+		return -EINVAL;
 
 	config.name = cdev->name;
 	config.dev = cdev->dev;
@@ -32,5 +38,18 @@ struct nvmem_device *nvmem_partition_register(struct cdev *cdev)
 	config.reg_read = nvmem_cdev_read;
 	config.reg_write = nvmem_cdev_write;
 
-	return nvmem_register(&config);
+	return PTR_ERR_OR_ZERO(nvmem_register(&config));
 }
+
+static __maybe_unused struct of_device_id nvmem_cells_dt_ids[] = {
+	{ .compatible = "nvmem-cells", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, nvmem_cells_dt_ids);
+
+static struct driver nvmem_cells_driver = {
+	.name	= "nvmem_cells",
+	.probe	= nvmem_cells_probe,
+	.of_compatible = nvmem_cells_dt_ids,
+};
+device_platform_driver(nvmem_cells_driver);
diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index 70297da9d231..56933cc958f7 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -76,12 +76,6 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
 	new->device_node = node;
 	new->flags |= DEVFS_PARTITION_FROM_OF | DEVFS_PARTITION_FOR_FIXUP;
 
-	if (IS_ENABLED(CONFIG_NVMEM) && of_device_is_compatible(node, "nvmem-cells")) {
-		struct nvmem_device *nvmem = nvmem_partition_register(new);
-		if (IS_ERR(nvmem))
-			dev_warn(cdev->dev, "nvmem registeration failed: %pe\n", nvmem);
-	}
-
 out:
 	free(filename);
 
@@ -108,6 +102,8 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
 		of_parse_partition(cdev, n);
 	}
 
+	if (subnode)
+		of_platform_populate(subnode, NULL, cdev->dev);
 	return 0;
 }
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 41c636b3a4e0..c1765673eb23 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -47,7 +47,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
 struct nvmem_device *nvmem_regmap_register(struct regmap *regmap, const char *name);
 struct nvmem_device *nvmem_regmap_register_with_pp(struct regmap *regmap,
 		const char *name, nvmem_cell_post_process_t cell_post_process);
-struct nvmem_device *nvmem_partition_register(struct cdev *cdev);
 struct device *nvmem_device_get_device(struct nvmem_device *nvmem);
 
 #else
@@ -69,11 +68,6 @@ nvmem_regmap_register_with_pp(struct regmap *regmap, const char *name,
 	return ERR_PTR(-ENOSYS);
 }
 
-static inline struct nvmem_device *nvmem_partition_register(struct cdev *cdev)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
 static inline struct device *nvmem_device_get_device(struct nvmem_device *nvmem)
 {
 	return ERR_PTR(-ENOSYS);
-- 
2.39.5

[PATCH 3/5] environment: register barebox env OF driver regardless of /chosen

[Ahmad Fatoum]

Even if we don't have a /chosen node, a barebox,environment node may be
added later or it may not be under /chosen at all with the incoming new
binding.

Therefore register the driver unconditionally if we are on an OF system.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/barebox.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/of/barebox.c b/drivers/of/barebox.c
index ed5d171e43a8..65392697913b 100644
--- a/drivers/of/barebox.c
+++ b/drivers/of/barebox.c
@@ -108,13 +108,9 @@ static int barebox_of_driver_init(void)
 		return 0;
 
 	node = of_find_node_by_path("/chosen");
-	if (!node)
-		return 0;
+	if (node)
+		of_platform_populate(node, of_default_bus_match_table, NULL);
 
-	of_platform_populate(node, of_default_bus_match_table, NULL);
-
-	platform_driver_register(&environment_driver);
-
-	return 0;
+	return platform_driver_register(&environment_driver);
 }
 late_initcall(barebox_of_driver_init);
-- 
2.39.5

[PATCH 4/5] environment: add support for a single node barebox,environment binding

[Ahmad Fatoum]

While we have had our binding for a long time, it was never submitted
for upstream inclusion in the Linux DT bindings and it's unlikely to be
accepted as there are no other bindings that reference a fixed partition
in a property.

Linux has support for a u-boot,env binding, which is used by giving the
partition a compatible = "u-boot,env" property.

Let's support the same thing for barebox and allow barebox,environment
to be a subnode of a fixed-partitions node. We intentionally don't add
file-path support for now as the intention is to flesh out this binding
together with upstream.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 .../bindings/nvmem/barebox,environment.yaml   | 55 +++++++++++++++
 drivers/of/barebox.c                          | 67 ++++++++++---------
 2 files changed, 90 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/barebox,environment.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/barebox,environment.yaml b/Documentation/devicetree/bindings/nvmem/barebox,environment.yaml
new file mode 100644
index 000000000000..9923baa19b40
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/barebox,environment.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://barebox.org/schemas/nvmem/barebox,environment.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: barebox environment variables
+
+description: |
+  barebox uses its environment to store non-volatile variables, scripts
+  and arbitrary binary blobs.
+  By default, the built-in environment compiled into barebox is used, but
+  for development purposes, it's possible to override it at runtime.
+
+  Variables can not yet be defined as NVMEM device subnodes.
+
+maintainers:
+  - Ahmad Fatoum <a.fatoum@pengutronix.de>
+
+properties:
+  compatible:
+    const: barebox,environment
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - label
+  - reg
+
+allOf:
+  - $ref: partition.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+    partitions {
+        compatible = "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            reg = <0x0 0x100000>;
+            label = "barebox";
+            read-only;
+        };
+
+        partition@100000 {
+            compatible = "barebox,environment";
+            label = "env";
+            reg = <0x100000 0x10000>;
+        };
+    };
diff --git a/drivers/of/barebox.c b/drivers/of/barebox.c
index 65392697913b..85e356f5ac0f 100644
--- a/drivers/of/barebox.c
+++ b/drivers/of/barebox.c
@@ -15,27 +15,38 @@
 
 #define ENV_MNT_DIR "/boot"	/* If env on filesystem, where to mount */
 
-/* If dev describes a file on a fs, mount the fs and return a pointer
- * to the file's path.  Otherwise return an error code or NULL if the
- * device path should be used.
- * Does nothing in env in a file support isn't enabled.
- */
-static char *environment_check_mount(struct device *dev, const char *devpath)
+static char *environment_probe_1node_binding(struct device *dev)
+{
+	struct cdev *cdev;
+
+	cdev = cdev_by_device_node(dev->of_node);
+	if (!cdev)
+		return ERR_PTR(-EINVAL);
+
+	return basprintf("/dev/%s", cdev_name(cdev));
+}
+
+static char *environment_probe_2node_binding(struct device *dev)
 {
 	const char *filepath;
+	char *devpath;
 	int ret;
 
+	ret = of_find_path(dev->of_node, "device-path", &devpath,
+			   OF_FIND_PATH_FLAGS_BB);
+	if (ret)
+		goto out;
+
 	if (!IS_ENABLED(CONFIG_OF_BAREBOX_ENV_IN_FS))
-		return NULL;
+		return devpath;
 
 	ret = of_property_read_string(dev->of_node, "file-path", &filepath);
 	if (ret == -EINVAL) {
-		/* No file-path so just use device-path */
-		return NULL;
+		return devpath;
 	} else if (ret) {
 		/* file-path property exists, but has error */
 		dev_err(dev, "Problem with file-path property\n");
-		return ERR_PTR(ret);
+		goto out;
 	}
 
 	/* Get device env is on and mount it */
@@ -44,42 +55,34 @@ static char *environment_check_mount(struct device *dev, const char *devpath)
 	if (ret) {
 		dev_err(dev, "Failed to load environment: mount %s failed (%d)\n",
 			devpath, ret);
-		return ERR_PTR(ret);
+		goto out;
 	}
 
 	/* Set env to be in a file on the now mounted device */
 	dev_dbg(dev, "Loading default env from %s on device %s\n",
 		filepath, devpath);
 
-	return basprintf("%s/%s", ENV_MNT_DIR, filepath);
+out:
+	free(devpath);
+	return ret ? ERR_PTR(ret) : basprintf("%s/%s", ENV_MNT_DIR, filepath);
 }
 
 static int environment_probe(struct device *dev)
 {
-	char *devpath, *filepath;
-	int ret;
+	char *path;
 
-	ret = of_find_path(dev->of_node, "device-path", &devpath,
-			   OF_FIND_PATH_FLAGS_BB);
-	if (ret)
-		return ret;
-
-	/* Do we need to mount a fs and find env there? */
-	filepath = environment_check_mount(dev, devpath);
-	if (IS_ERR(filepath)) {
-		free(devpath);
-		return ret;
-	}
-
-	if (filepath)
-		free(devpath);
+	if (of_property_present(dev->of_node, "device-path"))
+		path = environment_probe_2node_binding(dev);
 	else
-		filepath = devpath;
+		path = environment_probe_1node_binding(dev);
 
-	dev_dbg(dev, "Setting default environment path to %s\n", filepath);
-	default_environment_path_set(filepath);
+	if (IS_ERR(path))
+		return PTR_ERR(path);
 
-	free(filepath);
+	dev_dbg(dev, "Setting default environment path to %s\n", path);
+	default_environment_path_set(path);
+
+	free(path);
 
 	return 0;
 }
-- 
2.39.5

[PATCH 5/5] sandbox: switch to new barebox environment binding

[Ahmad Fatoum]

To have an in-tree user for the barebox environment binding, let's
switch over the sandbox DT to use it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/sandbox/dts/sandbox.dts | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
index cf47513c76fe..ec00301d0980 100644
--- a/arch/sandbox/dts/sandbox.dts
+++ b/arch/sandbox/dts/sandbox.dts
@@ -12,13 +12,6 @@ aliases {
 		state = &state;
 	};
 
-	chosen {
-		environment {
-			compatible = "barebox,environment";
-			device-path = &part_env;
-		};
-	};
-
 	memory {
 		device_type = "memory";
 		reg = <0 0 0 0>;
@@ -132,6 +125,7 @@ reboot_mode: reboot-mode@1 {
 			};
 
 			part_env: env@400 {
+				compatible = "barebox,environment";
 				reg = <0x400 0x800>;
 				label = "env";
 			};
-- 
2.39.5

Re: [PATCH 0/5] environment: upstream-NVMEM-compliant OF binding

[Sascha Hauer]

On Tue, 26 Nov 2024 16:33:07 +0100, Ahmad Fatoum wrote:
> While we have had our binding for a long time, it was never submitted
> for upstream inclusion in the Linux DT bindings and it's unlikely to be
> accepted as there are no other bindings that reference a fixed partition
> in a property.
> 
> Linux has support for a u-boot,env binding, which is used by giving the
> partition a compatible = "u-boot,env" property.
> 
> [...]

Applied, thanks!

[1/5] of: partition: don't parse nvmem-cells in legacy OF binding
      https://git.pengutronix.de/cgit/barebox/commit/?id=02fc38464af2 (link may not be stable)
[2/5] nvmem: probe nvmem-cells container via driver model
      https://git.pengutronix.de/cgit/barebox/commit/?id=67dc0fae9496 (link may not be stable)
[3/5] environment: register barebox env OF driver regardless of /chosen
      https://git.pengutronix.de/cgit/barebox/commit/?id=64a14b10341a (link may not be stable)
[4/5] environment: add support for a single node barebox,environment binding
      https://git.pengutronix.de/cgit/barebox/commit/?id=2e5876b24293 (link may not be stable)
[5/5] sandbox: switch to new barebox environment binding
      https://git.pengutronix.de/cgit/barebox/commit/?id=6302b629aec6 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>