[PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

MTD subsystem API allows interacting with MTD devices (e.g. reading,
writing, handling bad blocks). So far a random driver could get MTD
device only by its name (get_mtd_device_nm()). This change allows
getting them also by a DT node.

This API is required for drivers handling DT defined MTD partitions in a
specific way (e.g. U-Boot (sub)partition with environment variables).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: First introduction of of_get_mtd_device_by_node()

mtd maintainers: please let know how would you like this patch
processed. Would that be OK for you to Review/Ack it and let it go
through NVMEM tree?
---
 drivers/mtd/mtdcore.c   | 28 ++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 9eb0680db312..7dc214271c85 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
 }
 EXPORT_SYMBOL_GPL(__get_mtd_device);
 
+/**
+ * of_get_mtd_device_by_node - obtain an MTD device associated with a given node
+ *
+ * @np: device tree node
+ */
+struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)
+{
+	struct mtd_info *mtd = NULL;
+	struct mtd_info *tmp;
+	int err;
+
+	mutex_lock(&mtd_table_mutex);
+
+	err = -ENODEV;
+	mtd_for_each_device(tmp) {
+		if (mtd_get_of_node(tmp) == np) {
+			mtd = tmp;
+			err = __get_mtd_device(mtd);
+			break;
+		}
+	}
+
+	mutex_unlock(&mtd_table_mutex);
+
+	return err ? ERR_PTR(err) : mtd;
+}
+EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
+
 /**
  *	get_mtd_device_nm - obtain a validated handle for an MTD device by
  *	device name
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 955aee14b0f7..6fc841ceef31 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -677,6 +677,7 @@ extern int mtd_device_unregister(struct mtd_info *master);
 extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern int __get_mtd_device(struct mtd_info *mtd);
 extern void __put_mtd_device(struct mtd_info *mtd);
+extern struct mtd_info *of_get_mtd_device_by_node(struct device_node *np);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
 extern void put_mtd_device(struct mtd_info *mtd);
 
-- 
2.34.1

[PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

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
often stored as U-Boot env variable.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: Use of_get_mtd_device_by_node() (thanks Ahmad) & update description
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@gmail.com/
---
 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@milecki.pl>
 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@linaro.org>
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.
+
 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@milecki.pl>
+ */
+
+#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];
+} __packed;
+
+struct u_boot_env_image_redundant {
+	__le32 crc32;
+	u8 mark;
+	uint8_t data[0];
+} __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);
+	if (err && !mtd_is_bitflip(err)) {
+		dev_err(dev, "Failed to read from mtd: %d\n", err);
+		return err;
+	}
+
+	if (bytes_read != bytes) {
+		dev_err(dev, "Failed to read %zd bytes\n", bytes);
+		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);
+	}
+
+	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;
+	priv->format = (uintptr_t)of_id->data;
+
+	if (of_property_read_u32(np, "reg", (u32 *)&priv->offset) ||
+	    of_property_read_u32_index(np, "reg", 1, (u32 *)&priv->size)) {
+		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));
+		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);
-- 
2.34.1

Re: [PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[Miquel Raynal]

Hi Rafał,

zajec5@gmail.com wrote on Sat, 11 Jun 2022 22:46:50 +0200:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> MTD subsystem API allows interacting with MTD devices (e.g. reading,
> writing, handling bad blocks). So far a random driver could get MTD
> device only by its name (get_mtd_device_nm()). This change allows
> getting them also by a DT node.
> 
> This API is required for drivers handling DT defined MTD partitions in a
> specific way (e.g. U-Boot (sub)partition with environment variables).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V3: First introduction of of_get_mtd_device_by_node()
> 
> mtd maintainers: please let know how would you like this patch
> processed. Would that be OK for you to Review/Ack it and let it go
> through NVMEM tree?

Yes

> ---
>  drivers/mtd/mtdcore.c   | 28 ++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 9eb0680db312..7dc214271c85 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
>  }
>  EXPORT_SYMBOL_GPL(__get_mtd_device);
>  
> +/**
> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given node
> + *
> + * @np: device tree node
> + */
> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)

Shall we try to use a more of-agnostic syntax or is it too complex here?

> +{
> +	struct mtd_info *mtd = NULL;
> +	struct mtd_info *tmp;
> +	int err;
> +
> +	mutex_lock(&mtd_table_mutex);
> +
> +	err = -ENODEV;
> +	mtd_for_each_device(tmp) {
> +		if (mtd_get_of_node(tmp) == np) {
> +			mtd = tmp;
> +			err = __get_mtd_device(mtd);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&mtd_table_mutex);
> +
> +	return err ? ERR_PTR(err) : mtd;
> +}
> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
> +
>  /**
>   *	get_mtd_device_nm - obtain a validated handle for an MTD device by
>   *	device name
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 955aee14b0f7..6fc841ceef31 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -677,6 +677,7 @@ extern int mtd_device_unregister(struct mtd_info *master);
>  extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
>  extern int __get_mtd_device(struct mtd_info *mtd);
>  extern void __put_mtd_device(struct mtd_info *mtd);
> +extern struct mtd_info *of_get_mtd_device_by_node(struct device_node *np);
>  extern struct mtd_info *get_mtd_device_nm(const char *name);
>  extern void put_mtd_device(struct mtd_info *mtd);
>  


Thanks,
Miquèl

Re: [PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[Rafał Miłecki]

On 13.06.2022 16:04, Miquel Raynal wrote:
>> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
>>   }
>>   EXPORT_SYMBOL_GPL(__get_mtd_device);
>>   
>> +/**
>> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given node
>> + *
>> + * @np: device tree node
>> + */
>> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)
> 
> Shall we try to use a more of-agnostic syntax or is it too complex here?

I need some extra hint, please. This is how many similar functions look
like:

$ grep -E -r "(get|find).*_by_node" ./include/*
./include/drm/drm_mipi_dsi.h:struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
./include/drm/drm_mipi_dsi.h:struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
./include/linux/usb/phy.h:extern struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
./include/linux/usb/phy.h:static inline struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
./include/linux/extcon.h:struct extcon_dev *extcon_find_edev_by_node(struct device_node *node);
./include/linux/extcon.h:static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
./include/linux/of_net.h:extern struct net_device *of_find_net_device_by_node(struct device_node *np);
./include/linux/of_net.h:static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
./include/linux/devfreq.h:struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node);
./include/linux/devfreq.h:static inline struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node)
./include/linux/of_platform.h:extern struct platform_device *of_find_device_by_node(struct device_node *np);
./include/linux/of_platform.h:static inline struct platform_device *of_find_device_by_node(struct device_node *np)
./include/linux/backlight.h:struct backlight_device *of_find_backlight_by_node(struct device_node *node);
./include/linux/backlight.h:of_find_backlight_by_node(struct device_node *node)
./include/linux/i2c.h:struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
./include/linux/i2c.h:struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
./include/linux/i2c.h:struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
./include/linux/i2c.h:static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
./include/linux/i2c.h:static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
./include/linux/i2c.h:static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)


>> +{
>> +	struct mtd_info *mtd = NULL;
>> +	struct mtd_info *tmp;
>> +	int err;
>> +
>> +	mutex_lock(&mtd_table_mutex);
>> +
>> +	err = -ENODEV;
>> +	mtd_for_each_device(tmp) {
>> +		if (mtd_get_of_node(tmp) == np) {
>> +			mtd = tmp;
>> +			err = __get_mtd_device(mtd);
>> +			break;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&mtd_table_mutex);
>> +
>> +	return err ? ERR_PTR(err) : mtd;
>> +}
>> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
>> +
>>   /**
>>    *	get_mtd_device_nm - obtain a validated handle for an MTD device by
>>    *	device name

Re: [PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[Miquel Raynal]

Hi Rafał,

zajec5@gmail.com wrote on Mon, 13 Jun 2022 16:15:34 +0200:

> On 13.06.2022 16:04, Miquel Raynal wrote:
> >> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
> >>   }
> >>   EXPORT_SYMBOL_GPL(__get_mtd_device);  
> >>   >> +/**  
> >> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given node
> >> + *
> >> + * @np: device tree node
> >> + */
> >> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)  
> > 
> > Shall we try to use a more of-agnostic syntax or is it too complex here?  
> 
> I need some extra hint, please. This is how many similar functions look
> like:

I know most implementation today use of_ functions directly but it
seems like there is a global move towards fwnodes now, and I was
wondering if using those instead (which might also apply to other types
of "nodes" than DT ones) could be possible.

But looking into existing implementations, I came across the pwm implem
which features:
- of_pwm_get()
- acpi_pwm_get()

And finally a fwnode_pwm_get() which does:

	if (is_of_node())
		of_pwm_get():
	else if (is_acpi_node())
		acpi_pwm_get();

So actually my suggestion is meaningless. I'm fine with the current
approach.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


> 
> $ grep -E -r "(get|find).*_by_node" ./include/*
> ./include/drm/drm_mipi_dsi.h:struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
> ./include/drm/drm_mipi_dsi.h:struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
> ./include/linux/usb/phy.h:extern struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
> ./include/linux/usb/phy.h:static inline struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
> ./include/linux/extcon.h:struct extcon_dev *extcon_find_edev_by_node(struct device_node *node);
> ./include/linux/extcon.h:static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
> ./include/linux/of_net.h:extern struct net_device *of_find_net_device_by_node(struct device_node *np);
> ./include/linux/of_net.h:static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
> ./include/linux/devfreq.h:struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node);
> ./include/linux/devfreq.h:static inline struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node)
> ./include/linux/of_platform.h:extern struct platform_device *of_find_device_by_node(struct device_node *np);
> ./include/linux/of_platform.h:static inline struct platform_device *of_find_device_by_node(struct device_node *np)
> ./include/linux/backlight.h:struct backlight_device *of_find_backlight_by_node(struct device_node *node);
> ./include/linux/backlight.h:of_find_backlight_by_node(struct device_node *node)
> ./include/linux/i2c.h:struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
> ./include/linux/i2c.h:struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
> ./include/linux/i2c.h:struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
> ./include/linux/i2c.h:static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> ./include/linux/i2c.h:static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> ./include/linux/i2c.h:static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
> 
> 
> >> +{
> >> +	struct mtd_info *mtd = NULL;
> >> +	struct mtd_info *tmp;
> >> +	int err;
> >> +
> >> +	mutex_lock(&mtd_table_mutex);
> >> +
> >> +	err = -ENODEV;
> >> +	mtd_for_each_device(tmp) {
> >> +		if (mtd_get_of_node(tmp) == np) {
> >> +			mtd = tmp;
> >> +			err = __get_mtd_device(mtd);
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	mutex_unlock(&mtd_table_mutex);
> >> +
> >> +	return err ? ERR_PTR(err) : mtd;
> >> +}
> >> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
> >> +
> >>   /**
> >>    *	get_mtd_device_nm - obtain a validated handle for an MTD device by
> >>    *	device name  
> 


Thanks,
Miquèl

Re: [PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables

[Ahmad Fatoum]

Hello Rafał,

On 11.06.22 22:46, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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@milecki.pl>
> ---
> 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@gmail.com/

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@milecki.pl>
>  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@linaro.org>
> 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@milecki.pl>
> + */
> +
> +#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 |

Re: [PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node

[Ahmad Fatoum]

Hello Rafał,

On 11.06.22 22:46, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> MTD subsystem API allows interacting with MTD devices (e.g. reading,
> writing, handling bad blocks). So far a random driver could get MTD
> device only by its name (get_mtd_device_nm()). This change allows
> getting them also by a DT node.
> 
> This API is required for drivers handling DT defined MTD partitions in a
> specific way (e.g. U-Boot (sub)partition with environment variables).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V3: First introduction of of_get_mtd_device_by_node()
> 
> mtd maintainers: please let know how would you like this patch
> processed. Would that be OK for you to Review/Ack it and let it go
> through NVMEM tree?
> ---
>  drivers/mtd/mtdcore.c   | 28 ++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 9eb0680db312..7dc214271c85 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
>  }
>  EXPORT_SYMBOL_GPL(__get_mtd_device);
>  
> +/**
> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given node
> + *
> + * @np: device tree node
> + */
> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)
> +{
> +	struct mtd_info *mtd = NULL;
> +	struct mtd_info *tmp;
> +	int err;
> +
> +	mutex_lock(&mtd_table_mutex);
> +
> +	err = -ENODEV;

Shouldn't this be -EPROBE_DEFER? That way drivers making
use of this function can defer probe until the device
is probed.

> +	mtd_for_each_device(tmp) {
> +		if (mtd_get_of_node(tmp) == np) {
> +			mtd = tmp;
> +			err = __get_mtd_device(mtd);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&mtd_table_mutex);
> +
> +	return err ? ERR_PTR(err) : mtd;
> +}
> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
> +
>  /**
>   *	get_mtd_device_nm - obtain a validated handle for an MTD device by
>   *	device name
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 955aee14b0f7..6fc841ceef31 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -677,6 +677,7 @@ extern int mtd_device_unregister(struct mtd_info *master);
>  extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
>  extern int __get_mtd_device(struct mtd_info *mtd);
>  extern void __put_mtd_device(struct mtd_info *mtd);
> +extern struct mtd_info *of_get_mtd_device_by_node(struct device_node *np);
>  extern struct mtd_info *get_mtd_device_nm(const char *name);
>  extern void put_mtd_device(struct mtd_info *mtd);
>  


-- 
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 |

Re: [PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables

[Sascha Hauer]

On Sat, Jun 11, 2022 at 10:46:51PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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
> often stored as U-Boot env variable.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V3: Use of_get_mtd_device_by_node() (thanks Ahmad) & update description
> 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@gmail.com/
> ---
>  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@milecki.pl>
>  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@linaro.org>
> 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.
> +
>  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@milecki.pl>
> + */
> +
> +#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];
> +} __packed;
> +
> +struct u_boot_env_image_redundant {
> +	__le32 crc32;
> +	u8 mark;
> +	uint8_t data[0];
> +} __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);
> +	if (err && !mtd_is_bitflip(err)) {
> +		dev_err(dev, "Failed to read from mtd: %d\n", err);
> +		return err;
> +	}
> +
> +	if (bytes_read != bytes) {
> +		dev_err(dev, "Failed to read %zd bytes\n", bytes);
> +		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);
> +	}
> +
> +	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;
> +	priv->format = (uintptr_t)of_id->data;
> +
> +	if (of_property_read_u32(np, "reg", (u32 *)&priv->offset) ||
> +	    of_property_read_u32_index(np, "reg", 1, (u32 *)&priv->size)) {
> +		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));
> +		return PTR_ERR(priv->mtd);
> +	}

Partitions are mtd devices themselves and the mtd layer directly
associates these devices to their OF node, so it should be possible
to do a of_get_mtd_device_by_node(np) which gets you the partition.
You can use the whole mtd device then and do not have to fiddle with
reg properties, offsets and sizes in your driver yourself.

Sascha

-- 
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 |

Re: [PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables

[Miquel Raynal]

Hello,

> > +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;
> > +	priv->format = (uintptr_t)of_id->data;
> > +
> > +	if (of_property_read_u32(np, "reg", (u32 *)&priv->offset) ||
> > +	    of_property_read_u32_index(np, "reg", 1, (u32 *)&priv->size)) {
> > +		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));
> > +		return PTR_ERR(priv->mtd);
> > +	}  
> 
> Partitions are mtd devices themselves and the mtd layer directly
> associates these devices to their OF node, so it should be possible
> to do a of_get_mtd_device_by_node(np) which gets you the partition.
> You can use the whole mtd device then and do not have to fiddle with
> reg properties, offsets and sizes in your driver yourself.

Just for the record, there will be one mtd device per partition, but
the "full" mtd device will only exist if the configuration contains
CONFIG_MTD_PARTITIONED_MASTER.

Thanks,
Miquèl

Re: [PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables

[Rafał Miłecki]

On 14.06.2022 08:45, Ahmad Fatoum wrote:
>> +	if (err && !mtd_is_bitflip(err)) {
>> +		dev_err(dev, "Failed to read from mtd: %d\n", err);
> 
> Nitpick: %pe

My "err" variable in int, not a pointer (I don't use PTR_ERR()).


>> +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?

I believe it can't. In any case \0 is a always a separator.


>> +	}
>> +
>> +	if (WARN_ON(idx != priv->ncells))
>> +		priv->ncells = idx;
>> +
>> +	return 0;
>> +}