[PATCH 0/3] nvmem: add ONIE NVMEM cells provider

[PATCH 0/3] nvmem: add ONIE NVMEM cells provider

[Vadym Kochan]

This series adds cells parser for the ONIE TLV attributes which are
stored on NVMEM device. It adds possibility to read the mac address (and
other info) by other drivers.

Because ONIE stores info in TLV format it should be parsed first and
then register the cells. Current NVMEM API allows to register cell
table with known cell's offset which is not guaranteed in case of TLV.

To make it properly handled the NVMEM parser object is introduced. The
parser needs to be registered before target NVMEM device is registered.
During the registration of NVMEM device the parser is called to parse
the device's cells and reister the cell table.

Vadym Kochan (3):
  nvmem: core: introduce cells parser
  nvmem: add ONIE nvmem cells parser
  dt-bindings: nvmem: add description for ONIE cells parser

 .../bindings/nvmem/onie,nvmem-cells.txt       |  11 +
 drivers/nvmem/Kconfig                         |   9 +
 drivers/nvmem/Makefile                        |   3 +
 drivers/nvmem/core.c                          |  80 ++++
 drivers/nvmem/onie-cells.c                    | 370 ++++++++++++++++++
 include/linux/nvmem-provider.h                |  30 ++
 6 files changed, 503 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
 create mode 100644 drivers/nvmem/onie-cells.c

-- 
2.17.1

[PATCH 1/3] nvmem: core: introduce cells parser

[Vadym Kochan]

Currently NVMEM core does not allow to register cells for an already
registered nvmem device and requires that this should be done before.
But there might a driver which needs to parse the nvmem device and then
register a cells table.

Introduce nvmem_parser API which allows to register cells parser which
is called during nvmem device registration. During this stage the parser
can read the nvmem device and register the cells table.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
 drivers/nvmem/core.c           | 89 ++++++++++++++++++++++++++++++++++
 include/linux/nvmem-provider.h | 27 +++++++++++
 2 files changed, 116 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6cd3edb2eaf6..82a96032bc3f 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -55,6 +55,14 @@ struct nvmem_cell {
 	struct list_head	node;
 };
 
+struct nvmem_parser {
+	struct device_node	*nvmem_of;
+	struct kref		refcnt;
+	struct list_head	head;
+	nvmem_parse_t		cells_parse;
+	void *priv;
+};
+
 static DEFINE_MUTEX(nvmem_mutex);
 static DEFINE_IDA(nvmem_ida);
 
@@ -64,6 +72,9 @@ static LIST_HEAD(nvmem_cell_tables);
 static DEFINE_MUTEX(nvmem_lookup_mutex);
 static LIST_HEAD(nvmem_lookup_list);
 
+static DEFINE_MUTEX(nvmem_parser_mutex);
+static LIST_HEAD(nvmem_parser_list);
+
 static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 
 static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
@@ -571,6 +582,30 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 	return 0;
 }
 
+static struct nvmem_parser *
+__nvmem_parser_find_of(const struct nvmem_device *nvmem)
+{
+	struct nvmem_parser *parser;
+
+	list_for_each_entry(parser, &nvmem_parser_list, head) {
+		if (dev_of_node(nvmem->base_dev) == parser->nvmem_of)
+			return parser;
+	}
+
+	return NULL;
+}
+
+static void nvmem_cells_parse(struct nvmem_device *nvmem)
+{
+	struct nvmem_parser *parser;
+
+	mutex_lock(&nvmem_parser_mutex);
+	parser = __nvmem_parser_find_of(nvmem);
+	if (parser && parser->cells_parse)
+		parser->cells_parse(parser->priv, nvmem);
+	mutex_unlock(&nvmem_parser_mutex);
+}
+
 /**
  * nvmem_register() - Register a nvmem device for given nvmem_config.
  * Also creates a binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
@@ -674,6 +709,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			goto err_teardown_compat;
 	}
 
+	nvmem_cells_parse(nvmem);
+
 	rval = nvmem_add_cells_from_table(nvmem);
 	if (rval)
 		goto err_remove_cells;
@@ -1630,6 +1667,58 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
 }
 EXPORT_SYMBOL_GPL(nvmem_dev_name);
 
+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+	struct device_node *nvmem_of;
+	struct nvmem_parser *parser;
+	int err;
+
+	if (!config->cells_parse)
+		return ERR_PTR(-EINVAL);
+
+	if (!config->dev)
+		return ERR_PTR(-EINVAL);
+
+	nvmem_of = of_parse_phandle(dev_of_node(config->dev), "nvmem", 0);
+	if (!nvmem_of)
+		return ERR_PTR(-EINVAL);
+
+	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
+	if (!parser) {
+		err = -ENOMEM;
+		goto err_alloc;
+	}
+
+	parser->cells_parse = config->cells_parse;
+	/* parser->cells_remove = config->cells_remove; */
+	parser->nvmem_of = nvmem_of;
+	parser->priv = config->priv;
+	kref_init(&parser->refcnt);
+
+	mutex_lock(&nvmem_parser_mutex);
+	list_add(&parser->head, &nvmem_parser_list);
+	mutex_unlock(&nvmem_parser_mutex);
+
+	return parser;
+
+err_alloc:
+	of_node_put(nvmem_of);
+
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL(nvmem_parser_register);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser)
+{
+	mutex_lock(&nvmem_parser_mutex);
+	of_node_put(parser->nvmem_of);
+	list_del(&parser->head);
+	kfree(parser);
+	mutex_unlock(&nvmem_parser_mutex);
+}
+EXPORT_SYMBOL(nvmem_parser_unregister);
+
 static int __init nvmem_init(void)
 {
 	return bus_register(&nvmem_bus_type);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 06409a6c40bc..854d0cf5234f 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -15,10 +15,13 @@
 
 struct nvmem_device;
 struct nvmem_cell_info;
+struct nvmem_cell_table;
+
 typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
 				void *val, size_t bytes);
 typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
 				 void *val, size_t bytes);
+typedef void (*nvmem_parse_t)(void *priv, struct nvmem_device *nvmem);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
@@ -100,6 +103,12 @@ struct nvmem_cell_table {
 	struct list_head	node;
 };
 
+struct nvmem_parser_config {
+	nvmem_parse_t	cells_parse;
+	void		*priv;
+	struct device	*dev;
+};
+
 #if IS_ENABLED(CONFIG_NVMEM)
 
 struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
@@ -110,9 +119,19 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
 
 int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
 
+int nvmem_cell_parser_register(const char *nvmem_name,
+			       const struct nvmem_config *cfg);
+void nvmem_cell_parser_unregister(const char *nvmem_name,
+				  const struct nvmem_config *cfg);
+
 void nvmem_add_cell_table(struct nvmem_cell_table *table);
 void nvmem_del_cell_table(struct nvmem_cell_table *table);
 
+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser);
+
 #else
 
 static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -137,5 +156,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
 static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
 static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
 
+static inline struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {}
+
 #endif /* CONFIG_NVMEM */
 #endif  /* ifndef _LINUX_NVMEM_PROVIDER_H */
-- 
2.17.1

[PATCH 2/3] nvmem: add ONIE nvmem cells parser

[Vadym Kochan]

ONIE is a small operating system, pre-installed on bare metal network
switches, that provides an environment for automated provisioning.

This system requires that NVMEM (EEPROM) device holds various system
information (mac address, platform name, etc) in a special TLV layout.

The driver parses ONIE TLV attributes and registers them as NVMEM cells
which can be accessed by other platform driver. Also it allows to use
of_get_mac_address() to retrieve mac address for the netdev.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
 drivers/nvmem/Kconfig      |   9 +
 drivers/nvmem/Makefile     |   3 +
 drivers/nvmem/onie-cells.c | 348 +++++++++++++++++++++++++++++++++++++
 3 files changed, 360 insertions(+)
 create mode 100644 drivers/nvmem/onie-cells.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 954d3b4a52ab..0e27caf4934e 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -270,4 +270,13 @@ config SPRD_EFUSE
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem-sprd-efuse.
 
+config NVMEM_ONIE_CELLS
+	tristate "ONIE TLV cells support"
+	help
+	  This is a driver to provide cells from ONIE TLV structure stored
+	  on NVMEM device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-onie-cells.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index a7c377218341..2199784a489f 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -55,3 +55,6 @@ obj-$(CONFIG_NVMEM_ZYNQMP)	+= nvmem_zynqmp_nvmem.o
 nvmem_zynqmp_nvmem-y		:= zynqmp_nvmem.o
 obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
 nvmem_sprd_efuse-y		:= sprd-efuse.o
+
+obj-$(CONFIG_NVMEM_ONIE_CELLS)	+= nvmem-onie-cells.o
+nvmem-onie-cells-y		:= onie-cells.o
diff --git a/drivers/nvmem/onie-cells.c b/drivers/nvmem/onie-cells.c
new file mode 100644
index 000000000000..4696050e6568
--- /dev/null
+++ b/drivers/nvmem/onie-cells.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ONIE NVMEM cells provider
+ *
+ * Author: Vadym Kochan <vadym.kochan@plvision.eu>
+ */
+
+#define ONIE_NVMEM_DRVNAME	"onie-nvmem-cells"
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define ONIE_NVMEM_TLV_MAX_LEN	2048
+
+#define ONIE_NVMEM_HDR_ID	"TlvInfo"
+
+struct onie_nvmem_hdr {
+	u8 id[8];
+	u8 version;
+	__be16 data_len;
+} __packed;
+
+struct onie_nvmem_tlv {
+	u8 type;
+	u8 len;
+	u8 val[0];
+} __packed;
+
+struct onie_nvmem_attr {
+	struct list_head head;
+	const char *name;
+	unsigned int offset;
+	unsigned int len;
+};
+
+struct onie_nvmem {
+	unsigned int attr_count;
+	struct list_head attrs;
+	struct device *dev;
+
+	struct nvmem_cell_lookup *lookup;
+	struct nvmem_cell_table table;
+	struct nvmem_parser *parser;
+};
+
+static bool onie_nvmem_hdr_is_valid(struct onie_nvmem_hdr *hdr)
+{
+	if (memcmp(hdr->id, ONIE_NVMEM_HDR_ID, sizeof(hdr->id)) != 0)
+		return false;
+	if (hdr->version != 0x1)
+		return false;
+
+	return true;
+}
+
+static void onie_nvmem_attrs_free(struct onie_nvmem *onie)
+{
+	struct onie_nvmem_attr *attr, *tmp;
+
+	list_for_each_entry_safe(attr, tmp, &onie->attrs, head) {
+		list_del(&attr->head);
+		kfree(attr);
+	}
+}
+
+static const char *onie_nvmem_attr_name(u8 type)
+{
+	switch (type) {
+	case 0x21: return "product-name";
+	case 0x22: return "part-number";
+	case 0x23: return "serial-number";
+	case 0x24: return "mac-address";
+	case 0x25: return "manufacture-date";
+	case 0x26: return "device-version";
+	case 0x27: return "label-revision";
+	case 0x28: return "platforn-name";
+	case 0x29: return "onie-version";
+	case 0x2A: return "num-macs";
+	case 0x2B: return "manufacturer";
+	case 0x2C: return "country-code";
+	case 0x2D: return "vendor";
+	case 0x2E: return "diag-version";
+	case 0x2F: return "service-tag";
+	case 0xFD: return "vendor-extension";
+	case 0xFE: return "crc32";
+
+	default: return "unknown";
+	}
+}
+
+static int onie_nvmem_tlv_parse(struct onie_nvmem *onie, u8 *data, u16 len)
+{
+	unsigned int hlen = sizeof(struct onie_nvmem_hdr);
+	unsigned int offset = 0;
+	int err;
+
+	onie->attr_count = 0;
+
+	while (offset < len) {
+		struct onie_nvmem_attr *attr;
+		struct onie_nvmem_tlv *tlv;
+
+		tlv = (struct onie_nvmem_tlv *)(data + offset);
+
+		if (offset + tlv->len >= len) {
+			dev_err(onie->dev, "TLV len is too big(0x%x) at 0x%x\n",
+				tlv->len, hlen + offset);
+
+			/* return success in case something was parsed */
+			return 0;
+		}
+
+		attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+		if (!attr) {
+			err = -ENOMEM;
+			goto err_attr_alloc;
+		}
+
+		attr->name = onie_nvmem_attr_name(tlv->type);
+		/* skip 'type' and 'len' */
+		attr->offset = hlen + offset + 2;
+		attr->len = tlv->len;
+
+		list_add(&attr->head, &onie->attrs);
+		onie->attr_count++;
+
+		offset += sizeof(*tlv) + tlv->len;
+	}
+
+	if (!onie->attr_count)
+		return -EINVAL;
+
+	return 0;
+
+err_attr_alloc:
+	onie_nvmem_attrs_free(onie);
+	return err;
+}
+
+static int
+onie_nvmem_decode(struct onie_nvmem *onie, struct nvmem_device *nvmem)
+{
+	struct onie_nvmem_hdr hdr;
+	u8 *data;
+	u16 len;
+	int ret;
+
+	ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+	if (ret < 0)
+		return ret;
+
+	if (!onie_nvmem_hdr_is_valid(&hdr)) {
+		dev_err(onie->dev, "invalid ONIE TLV header\n");
+		return -EINVAL;
+	}
+
+	len = be16_to_cpu(hdr.data_len);
+
+	if (len > ONIE_NVMEM_TLV_MAX_LEN)
+		len = ONIE_NVMEM_TLV_MAX_LEN;
+
+	data = kmalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = nvmem_device_read(nvmem, sizeof(hdr), len, data);
+	if (ret < 0)
+		goto err_data_read;
+
+	ret = onie_nvmem_tlv_parse(onie, data, len);
+	if (ret)
+		goto err_info_parse;
+
+	kfree(data);
+
+	return 0;
+
+err_info_parse:
+err_data_read:
+	kfree(data);
+	return ret;
+}
+
+static int onie_nvmem_cells_parse(struct onie_nvmem *onie,
+				  struct nvmem_device *nvmem,
+				  struct nvmem_cell_table *table)
+{
+	struct nvmem_cell_info *cells;
+	struct onie_nvmem_attr *attr;
+	unsigned int ncells = 0;
+	int err;
+
+	INIT_LIST_HEAD(&onie->attrs);
+	onie->attr_count = 0;
+
+	err = onie_nvmem_decode(onie, nvmem);
+	if (err)
+		return err;
+
+	cells = kmalloc_array(onie->attr_count, sizeof(*cells), GFP_KERNEL);
+	if (!cells) {
+		err = -ENOMEM;
+		goto err_cells_alloc;
+	}
+
+	onie->lookup = kmalloc_array(onie->attr_count,
+				     sizeof(struct nvmem_cell_lookup),
+				     GFP_KERNEL);
+	if (!onie->lookup) {
+		err = -ENOMEM;
+		goto err_lookup_alloc;
+	}
+
+	list_for_each_entry(attr, &onie->attrs, head) {
+		struct nvmem_cell_lookup *lookup;
+		struct nvmem_cell_info *cell;
+
+		cell = &cells[ncells];
+
+		lookup = &onie->lookup[ncells];
+		lookup->con_id = NULL;
+
+		cell->offset = attr->offset;
+		cell->name = attr->name;
+		cell->bytes = attr->len;
+		cell->bit_offset = 0;
+		cell->nbits = 0;
+
+		lookup->nvmem_name = nvmem_dev_name(nvmem);
+		lookup->dev_id = dev_name(onie->dev);
+		lookup->cell_name = cell->name;
+		lookup->con_id = cell->name;
+
+		ncells++;
+	}
+
+	table->nvmem_name = nvmem_dev_name(nvmem);
+	table->ncells = ncells;
+	table->cells = cells;
+
+	onie_nvmem_attrs_free(onie);
+
+	return 0;
+
+err_lookup_alloc:
+	kfree(cells);
+err_cells_alloc:
+	onie_nvmem_attrs_free(onie);
+
+	return err;
+}
+
+static void onie_cells_parse(void *priv, struct nvmem_device *nvmem)
+{
+	struct onie_nvmem *onie = priv;
+	int err;
+
+	err = onie_nvmem_cells_parse(onie, nvmem, &onie->table);
+	if (err) {
+		dev_err(onie->dev, "failed to parse ONIE attributes\n");
+		return;
+	}
+
+	nvmem_add_cell_lookups(onie->lookup, onie->table.ncells);
+	nvmem_add_cell_table(&onie->table);
+}
+
+static int onie_nvmem_probe(struct platform_device *pdev)
+{
+	struct nvmem_parser_config parser_config = { };
+	struct device *dev = &pdev->dev;
+	struct onie_nvmem *onie;
+
+	onie = kzalloc(sizeof(*onie), GFP_KERNEL);
+	if (!onie)
+		return -ENOMEM;
+
+	onie->dev = dev;
+
+	dev_set_drvdata(dev, onie);
+
+	parser_config.cells_parse = onie_cells_parse;
+	parser_config.priv = onie;
+	parser_config.dev = dev;
+
+	onie->parser = nvmem_parser_register(&parser_config);
+	if (IS_ERR(onie->parser))
+		return PTR_ERR(onie->parser);
+
+	return 0;
+}
+
+static int onie_nvmem_remove(struct platform_device *pdev)
+{
+	struct onie_nvmem *onie = dev_get_drvdata(&pdev->dev);
+
+	if (onie->lookup)
+		nvmem_del_cell_lookups(onie->lookup, onie->attr_count);
+	if (onie->table.cells)
+		nvmem_del_cell_table(&onie->table);
+	nvmem_parser_unregister(onie->parser);
+	kfree(onie->table.cells);
+	kfree(onie->lookup);
+	kfree(onie);
+
+	return 0;
+}
+
+static const struct of_device_id onie_nvmem_match[] = {
+	{
+		.compatible = "onie-nvmem-cells",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, onie_nvmem_match);
+
+static struct platform_driver onie_nvmem_driver = {
+	.probe = onie_nvmem_probe,
+	.remove = onie_nvmem_remove,
+	.driver = {
+		.name = ONIE_NVMEM_DRVNAME,
+		.of_match_table = onie_nvmem_match,
+	},
+};
+
+static int __init onie_nvmem_init(void)
+{
+	return platform_driver_register(&onie_nvmem_driver);
+}
+
+static void __exit onie_nvmem_exit(void)
+{
+	platform_driver_unregister(&onie_nvmem_driver);
+}
+
+subsys_initcall(onie_nvmem_init);
+module_exit(onie_nvmem_exit);
+
+MODULE_AUTHOR("Vadym Kochan <vadym.kochan@plvision.eu>");
+MODULE_DESCRIPTION("ONIE NVMEM cells driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

[PATCH 3/3] dt-bindings: nvmem: add description for ONIE cells parser

[Vadym Kochan]

Add device-tree binding description for the ONIE cells parser.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
 .../devicetree/bindings/nvmem/onie-nvmem-cells.txt    | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt

diff --git a/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt b/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt
new file mode 100644
index 000000000000..db06d8b297b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt
@@ -0,0 +1,11 @@
+= Device tree bindings for ONIE cells parser =
+
+Required properties:
+- compatible: should be "onie-nvmem-cells"
+- nvmem: phandle to nvmem device node
+
+Example:
+	onie_cells {
+		compatible = "onie-nvmem-cells"
+		nvmem = <&at24>;
+	};
-- 
2.17.1

Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

[Vadym Kochan]

Hi Srinivas,

On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
> This series adds cells parser for the ONIE TLV attributes which are
> stored on NVMEM device. It adds possibility to read the mac address (and
> other info) by other drivers.
> 
> Because ONIE stores info in TLV format it should be parsed first and
> then register the cells. Current NVMEM API allows to register cell
> table with known cell's offset which is not guaranteed in case of TLV.
> 
> To make it properly handled the NVMEM parser object is introduced. The
> parser needs to be registered before target NVMEM device is registered.
> During the registration of NVMEM device the parser is called to parse
> the device's cells and reister the cell table.
> 
> Vadym Kochan (3):
>   nvmem: core: introduce cells parser
>   nvmem: add ONIE nvmem cells parser
>   dt-bindings: nvmem: add description for ONIE cells parser
> 
>  .../bindings/nvmem/onie,nvmem-cells.txt       |  11 +
>  drivers/nvmem/Kconfig                         |   9 +
>  drivers/nvmem/Makefile                        |   3 +
>  drivers/nvmem/core.c                          |  80 ++++
>  drivers/nvmem/onie-cells.c                    | 370 ++++++++++++++++++
>  include/linux/nvmem-provider.h                |  30 ++
>  6 files changed, 503 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
>  create mode 100644 drivers/nvmem/onie-cells.c
> 
> -- 
> 2.17.1
> 

I sent a newer version than this one which actually registers nvmem provider
and does not require changes in the core.c

Thanks,
Vadym Kochan

Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

[Srinivas Kandagatla]

On 22/09/2020 00:56, Vadym Kochan wrote:
> Hi Srinivas,
> 
> On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
>> This series adds cells parser for the ONIE TLV attributes which are
>> stored on NVMEM device. It adds possibility to read the mac address (and
>> other info) by other drivers.
>>
>> Because ONIE stores info in TLV format it should be parsed first and
>> then register the cells. Current NVMEM API allows to register cell
>> table with known cell's offset which is not guaranteed in case of TLV.
>>
>> To make it properly handled the NVMEM parser object is introduced. The
>> parser needs to be registered before target NVMEM device is registered.
>> During the registration of NVMEM device the parser is called to parse
>> the device's cells and reister the cell table.
>>
>> Vadym Kochan (3):
>>    nvmem: core: introduce cells parser
>>    nvmem: add ONIE nvmem cells parser
>>    dt-bindings: nvmem: add description for ONIE cells parser
>>
>>   .../bindings/nvmem/onie,nvmem-cells.txt       |  11 +
>>   drivers/nvmem/Kconfig                         |   9 +
>>   drivers/nvmem/Makefile                        |   3 +
>>   drivers/nvmem/core.c                          |  80 ++++
>>   drivers/nvmem/onie-cells.c                    | 370 ++++++++++++++++++
>>   include/linux/nvmem-provider.h                |  30 ++
>>   6 files changed, 503 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
>>   create mode 100644 drivers/nvmem/onie-cells.c
>>
>> -- 
>> 2.17.1
>>

Hi Vdaym,

Am totally confused with this patchset, There is no versioning in any of 
your patches, you always send it with PATCH, please add version so that 
I know which one should I review!

This makes my mailbox totally confused with all the patches with same 
subject prefix!

Please note that maintenance is not my full time job, so please be 
patient and I can try shift gears as an when possible!

> 
> I sent a newer version than this one which actually registers nvmem provider
> and does not require changes in the core.c
This is a NO-NO as onie is not a real provider here, at24 is the actual 
nvmem provider in your case.

Why do you keep changing the total approach here! what is the reasoning 
to do so!
As I said in my last review we were okay with this parser approach!

I don't mind having changes in core as long as it done properly!


thanks,
srini




> 
> Thanks,
> Vadym Kochan
> 

Re: [PATCH 1/3] nvmem: core: introduce cells parser

[Srinivas Kandagatla]

On 15/09/2020 13:41, Vadym Kochan wrote:
> Currently NVMEM core does not allow to register cells for an already
> registered nvmem device and requires that this should be done before.
> But there might a driver which needs to parse the nvmem device and then
> register a cells table.
> 
> Introduce nvmem_parser API which allows to register cells parser which
> is called during nvmem device registration. During this stage the parser
> can read the nvmem device and register the cells table.
> 

Overall this approach looks okay to me!

Rather than a binding for onie cell parser, I think we should add a 
generic bindings for parser something like:


nvmem-cell-parser = "onie-tlv-cells";


I also think the parser matching should be done based on this string 
which can remove
1. new DT node for onie parser.
2. works for both DT and non-DT setups.

nvmem provider register will automatically parse this once it finds a 
matching parser or it will EPROBE_DEFER!

Let me know if you think it works for you!


--srini

> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
>   drivers/nvmem/core.c           | 89 ++++++++++++++++++++++++++++++++++
>   include/linux/nvmem-provider.h | 27 +++++++++++
>   2 files changed, 116 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6cd3edb2eaf6..82a96032bc3f 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -55,6 +55,14 @@ struct nvmem_cell {
>   	struct list_head	node;
>   };
>   
> +struct nvmem_parser {
> +	struct device_node	*nvmem_of;
> +	struct kref		refcnt;
> +	struct list_head	head;
> +	nvmem_parse_t		cells_parse;
> +	void *priv;
> +};
> +
>   static DEFINE_MUTEX(nvmem_mutex);
>   static DEFINE_IDA(nvmem_ida);
>   
> @@ -64,6 +72,9 @@ static LIST_HEAD(nvmem_cell_tables);
>   static DEFINE_MUTEX(nvmem_lookup_mutex);
>   static LIST_HEAD(nvmem_lookup_list);
>   
> +static DEFINE_MUTEX(nvmem_parser_mutex);
> +static LIST_HEAD(nvmem_parser_list);
> +
>   static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>   
>   static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> @@ -571,6 +582,30 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>   	return 0;
>   }
>   
> +static struct nvmem_parser *
> +__nvmem_parser_find_of(const struct nvmem_device *nvmem)
> +{
> +	struct nvmem_parser *parser;
> +
> +	list_for_each_entry(parser, &nvmem_parser_list, head) {
> +		if (dev_of_node(nvmem->base_dev) == parser->nvmem_of)
> +			return parser;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void nvmem_cells_parse(struct nvmem_device *nvmem)
> +{
> +	struct nvmem_parser *parser;
> +
> +	mutex_lock(&nvmem_parser_mutex);
> +	parser = __nvmem_parser_find_of(nvmem);
> +	if (parser && parser->cells_parse)
> +		parser->cells_parse(parser->priv, nvmem);
> +	mutex_unlock(&nvmem_parser_mutex);
> +}
> +
>   /**
>    * nvmem_register() - Register a nvmem device for given nvmem_config.
>    * Also creates a binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
> @@ -674,6 +709,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   			goto err_teardown_compat;
>   	}
>   
> +	nvmem_cells_parse(nvmem);
> +
>   	rval = nvmem_add_cells_from_table(nvmem);
>   	if (rval)
>   		goto err_remove_cells;
> @@ -1630,6 +1667,58 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
>   }
>   EXPORT_SYMBOL_GPL(nvmem_dev_name);
>   
> +struct nvmem_parser *
> +nvmem_parser_register(const struct nvmem_parser_config *config)
> +{
> +	struct device_node *nvmem_of;
> +	struct nvmem_parser *parser;
> +	int err;
> +
> +	if (!config->cells_parse)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!config->dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	nvmem_of = of_parse_phandle(dev_of_node(config->dev), "nvmem", 0);
> +	if (!nvmem_of)
> +		return ERR_PTR(-EINVAL);
> +
> +	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> +	if (!parser) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	parser->cells_parse = config->cells_parse;
> +	/* parser->cells_remove = config->cells_remove; */
> +	parser->nvmem_of = nvmem_of;
> +	parser->priv = config->priv;
> +	kref_init(&parser->refcnt);
> +
> +	mutex_lock(&nvmem_parser_mutex);
> +	list_add(&parser->head, &nvmem_parser_list);
> +	mutex_unlock(&nvmem_parser_mutex);
> +
> +	return parser;
> +
> +err_alloc:
> +	of_node_put(nvmem_of);
> +
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(nvmem_parser_register);
> +
> +void nvmem_parser_unregister(struct nvmem_parser *parser)
> +{
> +	mutex_lock(&nvmem_parser_mutex);
> +	of_node_put(parser->nvmem_of);
> +	list_del(&parser->head);
> +	kfree(parser);
> +	mutex_unlock(&nvmem_parser_mutex);
> +}
> +EXPORT_SYMBOL(nvmem_parser_unregister);
> +
>   static int __init nvmem_init(void)
>   {
>   	return bus_register(&nvmem_bus_type);
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 06409a6c40bc..854d0cf5234f 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -15,10 +15,13 @@
>   
>   struct nvmem_device;
>   struct nvmem_cell_info;
> +struct nvmem_cell_table;
> +
>   typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
>   				void *val, size_t bytes);
>   typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
>   				 void *val, size_t bytes);
> +typedef void (*nvmem_parse_t)(void *priv, struct nvmem_device *nvmem);
>   
>   enum nvmem_type {
>   	NVMEM_TYPE_UNKNOWN = 0,
> @@ -100,6 +103,12 @@ struct nvmem_cell_table {
>   	struct list_head	node;
>   };
>   
> +struct nvmem_parser_config {
> +	nvmem_parse_t	cells_parse;
> +	void		*priv;
> +	struct device	*dev;
> +};
> +
>   #if IS_ENABLED(CONFIG_NVMEM)
>   
>   struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
> @@ -110,9 +119,19 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
>   
>   int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
>   
> +int nvmem_cell_parser_register(const char *nvmem_name,
> +			       const struct nvmem_config *cfg);
> +void nvmem_cell_parser_unregister(const char *nvmem_name,
> +				  const struct nvmem_config *cfg);
> +
>   void nvmem_add_cell_table(struct nvmem_cell_table *table);
>   void nvmem_del_cell_table(struct nvmem_cell_table *table);
>   
> +struct nvmem_parser *
> +nvmem_parser_register(const struct nvmem_parser_config *config);
> +
> +void nvmem_parser_unregister(struct nvmem_parser *parser);
> +
>   #else
>   
>   static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
> @@ -137,5 +156,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
>   static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
>   static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
>   
> +static inline struct nvmem_parser *
> +nvmem_parser_register(const struct nvmem_parser_config *config)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {}
> +
>   #endif /* CONFIG_NVMEM */
>   #endif  /* ifndef _LINUX_NVMEM_PROVIDER_H */
> 

Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

[Vadym Kochan]

Hi Srinivas,

On Tue, Sep 22, 2020 at 10:48:23AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 22/09/2020 00:56, Vadym Kochan wrote:
> > Hi Srinivas,
> > 
> > On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
> > > This series adds cells parser for the ONIE TLV attributes which are
> > > stored on NVMEM device. It adds possibility to read the mac address (and
> > > other info) by other drivers.
> > > 
> > > Because ONIE stores info in TLV format it should be parsed first and
> > > then register the cells. Current NVMEM API allows to register cell
> > > table with known cell's offset which is not guaranteed in case of TLV.
> > > 
> > > To make it properly handled the NVMEM parser object is introduced. The
> > > parser needs to be registered before target NVMEM device is registered.
> > > During the registration of NVMEM device the parser is called to parse
> > > the device's cells and reister the cell table.
> > > 
> > > Vadym Kochan (3):
> > >    nvmem: core: introduce cells parser
> > >    nvmem: add ONIE nvmem cells parser
> > >    dt-bindings: nvmem: add description for ONIE cells parser
> > > 
> > >   .../bindings/nvmem/onie,nvmem-cells.txt       |  11 +
> > >   drivers/nvmem/Kconfig                         |   9 +
> > >   drivers/nvmem/Makefile                        |   3 +
> > >   drivers/nvmem/core.c                          |  80 ++++
> > >   drivers/nvmem/onie-cells.c                    | 370 ++++++++++++++++++
> > >   include/linux/nvmem-provider.h                |  30 ++
> > >   6 files changed, 503 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
> > >   create mode 100644 drivers/nvmem/onie-cells.c
> > > 
> > > -- 
> > > 2.17.1
> > > 
> 
> Hi Vdaym,
> 
> Am totally confused with this patchset, There is no versioning in any of
> your patches, you always send it with PATCH, please add version so that I
> know which one should I review!
> 
> This makes my mailbox totally confused with all the patches with same
> subject prefix!
> 
> Please note that maintenance is not my full time job, so please be patient
> and I can try shift gears as an when possible!

Thank you!

> 
> > 
> > I sent a newer version than this one which actually registers nvmem provider
> > and does not require changes in the core.c
> This is a NO-NO as onie is not a real provider here, at24 is the actual
> nvmem provider in your case.
> 
> Why do you keep changing the total approach here! what is the reasoning to
> do so!

Well, I though that maybe anyway it also better to show the code, and
try different approaches.

> As I said in my last review we were okay with this parser approach!
> 
> I don't mind having changes in core as long as it done properly!
> 
> 
> thanks,
> srini

I am really sorry for this! I was really conceptually confused about how
to make it right by design. I will use "parser" word in next series
subject update with a versioning (v2) so you can identify that this is
related to parser approach with which you are conceptually fine.

Again sorry for this!

> 
> 
> 
> 
> > 
> > Thanks,
> > Vadym Kochan
> > 

Re: [PATCH 1/3] nvmem: core: introduce cells parser

[Vadym Kochan]

On Tue, Sep 22, 2020 at 10:48:27AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 15/09/2020 13:41, Vadym Kochan wrote:
> > Currently NVMEM core does not allow to register cells for an already
> > registered nvmem device and requires that this should be done before.
> > But there might a driver which needs to parse the nvmem device and then
> > register a cells table.
> > 
> > Introduce nvmem_parser API which allows to register cells parser which
> > is called during nvmem device registration. During this stage the parser
> > can read the nvmem device and register the cells table.
> > 
> 
> Overall this approach looks okay to me!
> 
> Rather than a binding for onie cell parser, I think we should add a generic
> bindings for parser something like:
> 
> 
> nvmem-cell-parser = "onie-tlv-cells";
> 
> 
> I also think the parser matching should be done based on this string which
> can remove
> 1. new DT node for onie parser.
> 2. works for both DT and non-DT setups.
> 
> nvmem provider register will automatically parse this once it finds a
> matching parser or it will EPROBE_DEFER!
> 
> Let me know if you think it works for you!
> 
> 
> --srini

Indeed, it sounds very good and fixes the early dependency that
parser should be registered first before nvmem provider.

I will try!

> 
> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> > ---
> >   drivers/nvmem/core.c           | 89 ++++++++++++++++++++++++++++++++++
> >   include/linux/nvmem-provider.h | 27 +++++++++++
> >   2 files changed, 116 insertions(+)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 6cd3edb2eaf6..82a96032bc3f 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -55,6 +55,14 @@ struct nvmem_cell {
> >   	struct list_head	node;
> >   };
> > +struct nvmem_parser {
> > +	struct device_node	*nvmem_of;
> > +	struct kref		refcnt;
> > +	struct list_head	head;
> > +	nvmem_parse_t		cells_parse;
> > +	void *priv;
> > +};
> > +
> >   static DEFINE_MUTEX(nvmem_mutex);
> >   static DEFINE_IDA(nvmem_ida);
> > @@ -64,6 +72,9 @@ static LIST_HEAD(nvmem_cell_tables);
> >   static DEFINE_MUTEX(nvmem_lookup_mutex);
> >   static LIST_HEAD(nvmem_lookup_list);
> > +static DEFINE_MUTEX(nvmem_parser_mutex);
> > +static LIST_HEAD(nvmem_parser_list);
> > +
> >   static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
> >   static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> > @@ -571,6 +582,30 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
> >   	return 0;
> >   }
> > +static struct nvmem_parser *
> > +__nvmem_parser_find_of(const struct nvmem_device *nvmem)
> > +{
> > +	struct nvmem_parser *parser;
> > +
> > +	list_for_each_entry(parser, &nvmem_parser_list, head) {
> > +		if (dev_of_node(nvmem->base_dev) == parser->nvmem_of)
> > +			return parser;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static void nvmem_cells_parse(struct nvmem_device *nvmem)
> > +{
> > +	struct nvmem_parser *parser;
> > +
> > +	mutex_lock(&nvmem_parser_mutex);
> > +	parser = __nvmem_parser_find_of(nvmem);
> > +	if (parser && parser->cells_parse)
> > +		parser->cells_parse(parser->priv, nvmem);
> > +	mutex_unlock(&nvmem_parser_mutex);
> > +}
> > +
> >   /**
> >    * nvmem_register() - Register a nvmem device for given nvmem_config.
> >    * Also creates a binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
> > @@ -674,6 +709,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >   			goto err_teardown_compat;
> >   	}
> > +	nvmem_cells_parse(nvmem);
> > +
> >   	rval = nvmem_add_cells_from_table(nvmem);
> >   	if (rval)
> >   		goto err_remove_cells;
> > @@ -1630,6 +1667,58 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
> >   }
> >   EXPORT_SYMBOL_GPL(nvmem_dev_name);
> > +struct nvmem_parser *
> > +nvmem_parser_register(const struct nvmem_parser_config *config)
> > +{
> > +	struct device_node *nvmem_of;
> > +	struct nvmem_parser *parser;
> > +	int err;
> > +
> > +	if (!config->cells_parse)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (!config->dev)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	nvmem_of = of_parse_phandle(dev_of_node(config->dev), "nvmem", 0);
> > +	if (!nvmem_of)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> > +	if (!parser) {
> > +		err = -ENOMEM;
> > +		goto err_alloc;
> > +	}
> > +
> > +	parser->cells_parse = config->cells_parse;
> > +	/* parser->cells_remove = config->cells_remove; */
> > +	parser->nvmem_of = nvmem_of;
> > +	parser->priv = config->priv;
> > +	kref_init(&parser->refcnt);
> > +
> > +	mutex_lock(&nvmem_parser_mutex);
> > +	list_add(&parser->head, &nvmem_parser_list);
> > +	mutex_unlock(&nvmem_parser_mutex);
> > +
> > +	return parser;
> > +
> > +err_alloc:
> > +	of_node_put(nvmem_of);
> > +
> > +	return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL(nvmem_parser_register);
> > +
> > +void nvmem_parser_unregister(struct nvmem_parser *parser)
> > +{
> > +	mutex_lock(&nvmem_parser_mutex);
> > +	of_node_put(parser->nvmem_of);
> > +	list_del(&parser->head);
> > +	kfree(parser);
> > +	mutex_unlock(&nvmem_parser_mutex);
> > +}
> > +EXPORT_SYMBOL(nvmem_parser_unregister);
> > +
> >   static int __init nvmem_init(void)
> >   {
> >   	return bus_register(&nvmem_bus_type);
> > diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> > index 06409a6c40bc..854d0cf5234f 100644
> > --- a/include/linux/nvmem-provider.h
> > +++ b/include/linux/nvmem-provider.h
> > @@ -15,10 +15,13 @@
> >   struct nvmem_device;
> >   struct nvmem_cell_info;
> > +struct nvmem_cell_table;
> > +
> >   typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
> >   				void *val, size_t bytes);
> >   typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
> >   				 void *val, size_t bytes);
> > +typedef void (*nvmem_parse_t)(void *priv, struct nvmem_device *nvmem);
> >   enum nvmem_type {
> >   	NVMEM_TYPE_UNKNOWN = 0,
> > @@ -100,6 +103,12 @@ struct nvmem_cell_table {
> >   	struct list_head	node;
> >   };
> > +struct nvmem_parser_config {
> > +	nvmem_parse_t	cells_parse;
> > +	void		*priv;
> > +	struct device	*dev;
> > +};
> > +
> >   #if IS_ENABLED(CONFIG_NVMEM)
> >   struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
> > @@ -110,9 +119,19 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
> >   int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
> > +int nvmem_cell_parser_register(const char *nvmem_name,
> > +			       const struct nvmem_config *cfg);
> > +void nvmem_cell_parser_unregister(const char *nvmem_name,
> > +				  const struct nvmem_config *cfg);
> > +
> >   void nvmem_add_cell_table(struct nvmem_cell_table *table);
> >   void nvmem_del_cell_table(struct nvmem_cell_table *table);
> > +struct nvmem_parser *
> > +nvmem_parser_register(const struct nvmem_parser_config *config);
> > +
> > +void nvmem_parser_unregister(struct nvmem_parser *parser);
> > +
> >   #else
> >   static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
> > @@ -137,5 +156,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
> >   static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
> >   static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
> > +static inline struct nvmem_parser *
> > +nvmem_parser_register(const struct nvmem_parser_config *config)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {}
> > +
> >   #endif /* CONFIG_NVMEM */
> >   #endif  /* ifndef _LINUX_NVMEM_PROVIDER_H */
> >