[RESEND RFC 0/3] Proposed extensions for NVMEM

[RESEND RFC 0/3] Proposed extensions for NVMEM

[Andrey Smirnov]

Hello,

This RFC introduces two new drivers to NVMEM subsytem. First driver,
'nvmem-blob', serves the purpose of exposing data, embedded in DTB,
via NVMEM consumer API. Second, 'nvmem-composite', allows the user to
combin a number of NVMEM cells (or parts of them) into a single
continuos "blob" of data presented to the rest of the system as a
regular NVMEM device.

The intent of this RFC is to solicit feedback about the approach,
binding or if these features should be accepted upstream in general.

I originally proposed this extentions as a part of Barebox project (Barebox
borrows a lot of concepts from Linux kernel and adopting NVMEM
subsystem was being discussed), in order to facilitate usage of NVMEM
to initialize source of MAC address for a paticular Ethernet adapter
(Barebox would read that value and fixup DT blob with appropriate
value as a part of booting Linux). However the drivers should be
generic enough to not be tied to that case.

Please bear in mind the the code included is a very rough draft and a
lot of error checking/handling code in is was stubbed out.

Any feedback is welcome.

Thank you,
Andrey Smirnov

Andrey Smirnov (3):
  nvmem: Add 'of_nvmem_cell_from_device_node()'
  nvmem: Add 'nvmem-blob' driver
  nvmem: Add 'nvmem-composite' driver

 Documentation/devicetree/bindings/nvmem/blob.txt   |  35 ++++
 .../devicetree/bindings/nvmem/composite.txt        |  44 ++++
 drivers/nvmem/Makefile                             |   2 +
 drivers/nvmem/blob.c                               | 132 ++++++++++++
 drivers/nvmem/composite.c                          | 228 +++++++++++++++++++++
 drivers/nvmem/core.c                               |  44 ++--
 include/linux/nvmem-consumer.h                     |   7 +
 7 files changed, 479 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/blob.txt
 create mode 100644 Documentation/devicetree/bindings/nvmem/composite.txt
 create mode 100644 drivers/nvmem/blob.c
 create mode 100644 drivers/nvmem/composite.c

-- 
2.5.0

[RESEND RFC 1/3] nvmem: Add 'of_nvmem_cell_from_device_node()'

[Andrey Smirnov]

Add 'of_nvmem_cell_from_device_node()' -- a function that allows to
obtain 'struct nvmem_cell' from a device tree node representing it. One
use-case for such a function would be to access nvmem cells with known
phandles.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/nvmem/core.c           | 44 +++++++++++++++++++++++++++++-------------
 include/linux/nvmem-consumer.h |  7 +++++++
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6fd4e5a..08550dd 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -601,29 +601,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
 /**
- * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
+ * of_nvmem_cell_from_device_node() - Get a nvmem cell from device node representation
  *
- * @dev node: Device tree node that uses the nvmem cell
- * @id: nvmem cell name from nvmem-cell-names property.
+ * @np node: Device tree node representing NVMEM cell
  *
  * Return: Will be an ERR_PTR() on error or a valid pointer
  * to a struct nvmem_cell.  The nvmem_cell will be freed by the
  * nvmem_cell_put().
  */
-struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
-					    const char *name)
+struct nvmem_cell *of_nvmem_cell_from_device_node(struct device_node *cell_np)
 {
-	struct device_node *cell_np, *nvmem_np;
+	struct device_node *nvmem_np;
 	struct nvmem_cell *cell;
 	struct nvmem_device *nvmem;
 	const __be32 *addr;
-	int rval, len, index;
-
-	index = of_property_match_string(np, "nvmem-cell-names", name);
-
-	cell_np = of_parse_phandle(np, "nvmem-cells", index);
-	if (!cell_np)
-		return ERR_PTR(-EINVAL);
+	int rval, len;
 
 	nvmem_np = of_get_next_parent(cell_np);
 	if (!nvmem_np)
@@ -682,6 +674,32 @@ err_mem:
 
 	return ERR_PTR(rval);
 }
+EXPORT_SYMBOL_GPL(of_nvmem_cell_from_device_node);
+
+/**
+ * of_nvmem_cell_get() - Get a nvmem cell from given device node referencing it and cell id
+ *
+ * @dev node: Device tree node that uses the nvmem cell
+ * @id: nvmem cell name from nvmem-cell-names property.
+ *
+ * Return: Will be an ERR_PTR() on error or a valid pointer
+ * to a struct nvmem_cell.  The nvmem_cell will be freed by the
+ * nvmem_cell_put().
+ */
+struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
+					    const char *name)
+{
+	struct device_node *cell_np;
+	int index;
+
+	index = of_property_match_string(np, "nvmem-cell-names", name);
+
+	cell_np = of_parse_phandle(np, "nvmem-cells", index);
+	if (!cell_np)
+		return ERR_PTR(-EINVAL);
+
+	return of_nvmem_cell_from_device_node(cell_np);
+}
 EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
 #endif
 
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 9bb77d3..ff0abb0 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -136,11 +136,18 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
 #endif /* CONFIG_NVMEM */
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
+struct nvmem_cell *of_nvmem_cell_from_device_node(struct device_node *np);
 struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name);
 struct nvmem_device *of_nvmem_device_get(struct device_node *np,
 					 const char *name);
 #else
+
+static inline struct nvmem_cell *
+of_nvmem_cell_from_device_node(struct device_node *np)
+{
+	return ERR_PTR(-ENOSYS);
+}
 static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name)
 {
-- 
2.5.0

Re: [RESEND RFC 1/3] nvmem: Add 'of_nvmem_cell_from_device_node()'

[Srinivas Kandagatla]

Sorry for so late review comments,


On 01/03/16 16:59, Andrey Smirnov wrote:
> Add 'of_nvmem_cell_from_device_node()' -- a function that allows to
> obtain 'struct nvmem_cell' from a device tree node representing it. One
> use-case for such a function would be to access nvmem cells with known
> phandles.

Totally missing the purpose of this new API, Why is of_nvmem_cell_get() 
not useful, its exactly doing same thing.

Unless you randomly want to handle phandles without proper dt bindings.

Thanks
srini

>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   drivers/nvmem/core.c           | 44 +++++++++++++++++++++++++++++-------------
>   include/linux/nvmem-consumer.h |  7 +++++++
>   2 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6fd4e5a..08550dd 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -601,29 +601,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>
>   #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
>   /**
> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
> + * of_nvmem_cell_from_device_node() - Get a nvmem cell from device node representation
>    *
> - * @dev node: Device tree node that uses the nvmem cell
> - * @id: nvmem cell name from nvmem-cell-names property.
> + * @np node: Device tree node representing NVMEM cell
>    *
>    * Return: Will be an ERR_PTR() on error or a valid pointer
>    * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>    * nvmem_cell_put().
>    */
> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> -					    const char *name)
> +struct nvmem_cell *of_nvmem_cell_from_device_node(struct device_node *cell_np)
>   {
> -	struct device_node *cell_np, *nvmem_np;
> +	struct device_node *nvmem_np;
>   	struct nvmem_cell *cell;
>   	struct nvmem_device *nvmem;
>   	const __be32 *addr;
> -	int rval, len, index;
> -
> -	index = of_property_match_string(np, "nvmem-cell-names", name);
> -
> -	cell_np = of_parse_phandle(np, "nvmem-cells", index);
> -	if (!cell_np)
> -		return ERR_PTR(-EINVAL);
> +	int rval, len;
>
>   	nvmem_np = of_get_next_parent(cell_np);
>   	if (!nvmem_np)
> @@ -682,6 +674,32 @@ err_mem:
>
>   	return ERR_PTR(rval);
>   }
> +EXPORT_SYMBOL_GPL(of_nvmem_cell_from_device_node);
> +
> +/**
> + * of_nvmem_cell_get() - Get a nvmem cell from given device node referencing it and cell id
> + *
> + * @dev node: Device tree node that uses the nvmem cell
> + * @id: nvmem cell name from nvmem-cell-names property.
> + *
> + * Return: Will be an ERR_PTR() on error or a valid pointer
> + * to a struct nvmem_cell.  The nvmem_cell will be freed by the
> + * nvmem_cell_put().
> + */
> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> +					    const char *name)
> +{
> +	struct device_node *cell_np;
> +	int index;
> +
> +	index = of_property_match_string(np, "nvmem-cell-names", name);
> +
> +	cell_np = of_parse_phandle(np, "nvmem-cells", index);
> +	if (!cell_np)
> +		return ERR_PTR(-EINVAL);
> +
> +	return of_nvmem_cell_from_device_node(cell_np);
> +}
>   EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>   #endif
>
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index 9bb77d3..ff0abb0 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -136,11 +136,18 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>   #endif /* CONFIG_NVMEM */
>
>   #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
> +struct nvmem_cell *of_nvmem_cell_from_device_node(struct device_node *np);
>   struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>   				     const char *name);
>   struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>   					 const char *name);
>   #else
> +
> +static inline struct nvmem_cell *
> +of_nvmem_cell_from_device_node(struct device_node *np)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
>   static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>   				     const char *name)
>   {
>

Re: [RESEND RFC 1/3] nvmem: Add 'of_nvmem_cell_from_device_node()'

[Andrey Smirnov]

On Wed, Mar 2, 2016 at 5:58 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Sorry for so late review comments,
>
>
> On 01/03/16 16:59, Andrey Smirnov wrote:
>>
>> Add 'of_nvmem_cell_from_device_node()' -- a function that allows to
>> obtain 'struct nvmem_cell' from a device tree node representing it. One
>> use-case for such a function would be to access nvmem cells with known
>> phandles.
>
>
> Totally missing the purpose of this new API, Why is of_nvmem_cell_get() not
> useful, its exactly doing same thing.
>
> Unless you randomly want to handle phandles without proper dt bindings.

That about sums up what I was trying to do. In my "composite" driver,
the way the layout of the cell is specified is a 3-element tuple
containing a phandle to nvmem cell, offset within that cell and the
size of the chunk to use, so in order to be able to use that phandle I
introduced this function.

Andrey

[RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Andrey Smirnov]

Add 'nvmem-blob' driver, which allows to access device tree embedded
data via NVMEM subsystem API.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 Documentation/devicetree/bindings/nvmem/blob.txt |  35 ++++++
 drivers/nvmem/Makefile                           |   1 +
 drivers/nvmem/blob.c                             | 132 +++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/blob.txt
 create mode 100644 drivers/nvmem/blob.c

diff --git a/Documentation/devicetree/bindings/nvmem/blob.txt b/Documentation/devicetree/bindings/nvmem/blob.txt
new file mode 100644
index 0000000..b299576
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/blob.txt
@@ -0,0 +1,35 @@
+= Deviece Tree Embedded Blob As NVMEM Device =
+
+This binding is designed to provide a way for a developer to reference
+data, built-in into device tree blob file, as NVMEM provider and acccess
+certain portions of that data as NVMEM cells using NVMEM consumer API.
+
+Required properties:
+- compatible: should be "nvmem-blob"
+- data: specifies data contained in nvmem device
+
+= Data cells =
+Are child nodes of nvmem-composite, bindings of which as described in
+bindings/nvmem/nvmem.txt
+
+Example:
+
+	a-blob {
+		compatible = "nvmem-blob";
+		data = [aa bb cc dd ee];
+
+		cell1: cell@0 {
+			reg = <0 5>;
+		};
+	};
+
+= Data consumers =
+Are device nodes which consume nvmem data cells.
+
+Example:
+
+	a-node {
+		...
+		nvmem-cells = <&cell1>;
+		nvmem-cell-names = "some-data";
+	};
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 95dde3f..1a1adba 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem_sunxi_sid.o
 nvmem_sunxi_sid-y		:= sunxi_sid.o
 obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
 nvmem-vf610-ocotp-y		:= vf610-ocotp.o
+obj-y += blob.o
diff --git a/drivers/nvmem/blob.c b/drivers/nvmem/blob.c
new file mode 100644
index 0000000..3f2296b
--- /dev/null
+++ b/drivers/nvmem/blob.c
@@ -0,0 +1,132 @@
+#define DEBUG 1
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct nvmem_blob {
+	const u8 *data;
+	size_t data_size;
+};
+
+static int nvmem_blob_write(void *context, const void *data,
+				size_t count)
+{
+	return -ENOTSUPP;
+}
+
+static int nvmem_blob_read(void *context,
+			   const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct nvmem_blob *nblob = context;
+	const unsigned int offset = *(u32 *)reg;
+	
+	memcpy(val, nblob->data + offset,
+	       min(val_size, nblob->data_size - offset));
+	return 0;
+}
+
+static const struct regmap_bus nvmem_blob_regmap_bus = {
+	.write = nvmem_blob_write,
+	.read  = nvmem_blob_read,
+};
+
+
+static int nvmem_blob_validate_dt(struct device_node *np)
+{
+	return 0;
+}
+
+static int nvmem_blob_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *map;
+	struct nvmem_device *nvmem;
+	struct nvmem_blob *nblob;
+	struct property *pp;
+	struct nvmem_config  nv_cnf = {0};
+	struct regmap_config rm_cnf = {0};
+
+	ret = nvmem_blob_validate_dt(np);
+	if (ret < 0) {
+		dev_dbg(dev, "Device tree validation failed\n");
+		return ret;
+	}
+
+	nblob = devm_kzalloc(dev, sizeof(*nblob), GFP_KERNEL);
+	if (!nblob) {
+		dev_dbg(dev, "Not enough memory to allocate a blob\n");
+		return -ENOMEM;
+	}
+
+	pp = of_find_property(np, "data", NULL);
+	BUG_ON(!pp);
+	
+	nblob->data = pp->value;
+	nblob->data_size = pp->length;
+
+	rm_cnf.reg_bits = 32;
+	rm_cnf.val_bits = 8;
+	rm_cnf.reg_stride = 1;
+	rm_cnf.name = "nvmem-blob";
+	rm_cnf.max_register = nblob->data_size - 1;
+
+	map = devm_regmap_init(dev,
+			       &nvmem_blob_regmap_bus,
+			       nblob,
+			       &rm_cnf);
+	if (IS_ERR(map)) {
+		dev_dbg(dev, "Failed to initilize regmap\n");
+		return PTR_ERR(map);
+	}
+
+	nv_cnf.name = "nvmem-blob";
+	nv_cnf.read_only = true;
+	nv_cnf.dev = dev;
+	nv_cnf.owner = THIS_MODULE;
+
+	nvmem = nvmem_register(&nv_cnf);
+	if (IS_ERR(nvmem)) {
+		dev_dbg(dev, "Filed to register nvmem device\n");
+		return PTR_ERR(nvmem);
+	}
+
+	platform_set_drvdata(pdev, nblob);
+	return 0;
+}
+
+static int nvmem_blob_remove(struct platform_device *pdev)
+{
+	struct nvmem_device *nvmem = platform_get_drvdata(pdev);
+
+	return nvmem_unregister(nvmem);
+}
+
+static const struct of_device_id nvmem_blob_dt_ids[] = {
+	{ .compatible = "nvmem-blob", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, nvmem_blob_dt_ids);
+
+static struct platform_driver nvmem_blob_driver = {
+	.probe	= nvmem_blob_probe,
+	.remove	= nvmem_blob_remove,
+	.driver = {
+		.name	= "nvmem-blob",
+		.of_match_table = nvmem_blob_dt_ids,
+	},
+};
+module_platform_driver(nvmem_blob_driver);
+
+MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
+MODULE_DESCRIPTION("FIXME");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Srinivas Kandagatla]

On 01/03/16 16:59, Andrey Smirnov wrote:
> Add 'nvmem-blob' driver, which allows to access device tree embedded
> data via NVMEM subsystem API.

Patch itself looks simple. Before we review it further could you provide 
more details on the exact usecase or some background of this.


Incase you resend the patches, could you split the dt bindings into 
separate patch and add dt guys in to the loop, They would have different 
opinion on adding dummy stuff into DT itself.

thanks,
srini
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   Documentation/devicetree/bindings/nvmem/blob.txt |  35 ++++++
>   drivers/nvmem/Makefile                           |   1 +
>   drivers/nvmem/blob.c                             | 132 +++++++++++++++++++++++
>   3 files changed, 168 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/nvmem/blob.txt
>   create mode 100644 drivers/nvmem/blob.c
>
> diff --git a/Documentation/devicetree/bindings/nvmem/blob.txt b/Documentation/devicetree/bindings/nvmem/blob.txt
> new file mode 100644
> index 0000000..b299576
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/blob.txt
> @@ -0,0 +1,35 @@
> += Deviece Tree Embedded Blob As NVMEM Device =
> +
> +This binding is designed to provide a way for a developer to reference
> +data, built-in into device tree blob file, as NVMEM provider and acccess
> +certain portions of that data as NVMEM cells using NVMEM consumer API.
> +
> +Required properties:
> +- compatible: should be "nvmem-blob"
> +- data: specifies data contained in nvmem device
> +
> += Data cells =
> +Are child nodes of nvmem-composite, bindings of which as described in
> +bindings/nvmem/nvmem.txt
> +
> +Example:
> +
> +	a-blob {
> +		compatible = "nvmem-blob";
> +		data = [aa bb cc dd ee];
> +
> +		cell1: cell@0 {
> +			reg = <0 5>;
> +		};
> +	};
> +
> += Data consumers =
> +Are device nodes which consume nvmem data cells.
> +
> +Example:
> +
> +	a-node {
> +		...
> +		nvmem-cells = <&cell1>;
> +		nvmem-cell-names = "some-data";
> +	};
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 95dde3f..1a1adba 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem_sunxi_sid.o
>   nvmem_sunxi_sid-y		:= sunxi_sid.o
>   obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
>   nvmem-vf610-ocotp-y		:= vf610-ocotp.o
> +obj-y += blob.o
> diff --git a/drivers/nvmem/blob.c b/drivers/nvmem/blob.c
> new file mode 100644
> index 0000000..3f2296b
> --- /dev/null
> +++ b/drivers/nvmem/blob.c
> @@ -0,0 +1,132 @@
> +#define DEBUG 1
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +struct nvmem_blob {
> +	const u8 *data;
> +	size_t data_size;
> +};
> +
> +static int nvmem_blob_write(void *context, const void *data,
> +				size_t count)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int nvmem_blob_read(void *context,
> +			   const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct nvmem_blob *nblob = context;
> +	const unsigned int offset = *(u32 *)reg;
> +	
> +	memcpy(val, nblob->data + offset,
> +	       min(val_size, nblob->data_size - offset));
> +	return 0;
> +}
> +
> +static const struct regmap_bus nvmem_blob_regmap_bus = {
> +	.write = nvmem_blob_write,
> +	.read  = nvmem_blob_read,
> +};
> +
> +
> +static int nvmem_blob_validate_dt(struct device_node *np)
> +{
> +	return 0;
> +}
> +
> +static int nvmem_blob_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct regmap *map;
> +	struct nvmem_device *nvmem;
> +	struct nvmem_blob *nblob;
> +	struct property *pp;
> +	struct nvmem_config  nv_cnf = {0};
> +	struct regmap_config rm_cnf = {0};
> +
> +	ret = nvmem_blob_validate_dt(np);
> +	if (ret < 0) {
> +		dev_dbg(dev, "Device tree validation failed\n");
> +		return ret;
> +	}
> +
> +	nblob = devm_kzalloc(dev, sizeof(*nblob), GFP_KERNEL);
> +	if (!nblob) {
> +		dev_dbg(dev, "Not enough memory to allocate a blob\n");
> +		return -ENOMEM;
> +	}
> +
> +	pp = of_find_property(np, "data", NULL);
> +	BUG_ON(!pp);
> +	
> +	nblob->data = pp->value;
> +	nblob->data_size = pp->length;
> +
> +	rm_cnf.reg_bits = 32;
> +	rm_cnf.val_bits = 8;
> +	rm_cnf.reg_stride = 1;
> +	rm_cnf.name = "nvmem-blob";
> +	rm_cnf.max_register = nblob->data_size - 1;
> +
> +	map = devm_regmap_init(dev,
> +			       &nvmem_blob_regmap_bus,
> +			       nblob,
> +			       &rm_cnf);
> +	if (IS_ERR(map)) {
> +		dev_dbg(dev, "Failed to initilize regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	nv_cnf.name = "nvmem-blob";
> +	nv_cnf.read_only = true;
> +	nv_cnf.dev = dev;
> +	nv_cnf.owner = THIS_MODULE;
> +
> +	nvmem = nvmem_register(&nv_cnf);
> +	if (IS_ERR(nvmem)) {
> +		dev_dbg(dev, "Filed to register nvmem device\n");
> +		return PTR_ERR(nvmem);
> +	}
> +
> +	platform_set_drvdata(pdev, nblob);
> +	return 0;
> +}
> +
> +static int nvmem_blob_remove(struct platform_device *pdev)
> +{
> +	struct nvmem_device *nvmem = platform_get_drvdata(pdev);
> +
> +	return nvmem_unregister(nvmem);
> +}
> +
> +static const struct of_device_id nvmem_blob_dt_ids[] = {
> +	{ .compatible = "nvmem-blob", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, nvmem_blob_dt_ids);
> +
> +static struct platform_driver nvmem_blob_driver = {
> +	.probe	= nvmem_blob_probe,
> +	.remove	= nvmem_blob_remove,
> +	.driver = {
> +		.name	= "nvmem-blob",
> +		.of_match_table = nvmem_blob_dt_ids,
> +	},
> +};
> +module_platform_driver(nvmem_blob_driver);
> +
> +MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
> +MODULE_DESCRIPTION("FIXME");
> +MODULE_LICENSE("GPL v2");
>

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Andrey Smirnov]

On Wed, Mar 2, 2016 at 5:58 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> On 01/03/16 16:59, Andrey Smirnov wrote:
>>
>> Add 'nvmem-blob' driver, which allows to access device tree embedded
>> data via NVMEM subsystem API.
>
>
> Patch itself looks simple. Before we review it further could you provide
> more details on the exact usecase or some background of this.

The discussion on this topic originated on mailing list of Barebox
project(which borrows very heavily from Linux designs). Barebox
operates on two device tree blobs, one is used for its internal
initialization, whereas second one is passed to Linux kernel when
booting it. The problem I was trying to solve was to make possible to
specify in the first DT blob what data would be used for MAC address
fixup of the second DT blob(the one passed to Linux).

My first approach was to implement a very limited DT code, however in
discussing it the consensus was that porting 'nvmem' subsystem from
the kernel and using for the same purpose would be a better approach.
First pass adoption of that subsystem revealed that there were two
use-cases that current design didn't allow us to handle:

- Depending on the version i.MX SoC MAC address data stored in ROM
would have different layout so as a possible solution to that I
implemented "composite" driver(patch #3)

- On i.MX28, part of the MAC address is hard-coded in
arch/arm/mach-mxs/mach-mxs.c and only a portion of it is read from
ROM, this patch in combination with the aforementioned one should
allow us to encode all needed info in DT.

Ideally, since all of the above is as applicable to Linux as it is to
Barebox it would be good for BB not to invent its own custom 'nvmem'
flavor, so hence me trying to start a conversation about adding this
upstream.

Hope this clarifies things a bit.

>
>
> Incase you resend the patches, could you split the dt bindings into separate
> patch and add dt guys in to the loop, They would have different opinion on
> adding dummy stuff into DT itself.

Will do.


Thanks,
Andrey

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

Hi,

On Wed, Mar 02, 2016 at 09:21:01AM -0800, Andrey Smirnov wrote:
> On Wed, Mar 2, 2016 at 5:58 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
> >
> >
> > On 01/03/16 16:59, Andrey Smirnov wrote:
> >>
> >> Add 'nvmem-blob' driver, which allows to access device tree embedded
> >> data via NVMEM subsystem API.
> >
> >
> > Patch itself looks simple. Before we review it further could you provide
> > more details on the exact usecase or some background of this.
> 
> The discussion on this topic originated on mailing list of Barebox
> project(which borrows very heavily from Linux designs). Barebox
> operates on two device tree blobs, one is used for its internal
> initialization, whereas second one is passed to Linux kernel when
> booting it. The problem I was trying to solve was to make possible to
> specify in the first DT blob what data would be used for MAC address
> fixup of the second DT blob(the one passed to Linux).
> 
> My first approach was to implement a very limited DT code, however in
> discussing it the consensus was that porting 'nvmem' subsystem from
> the kernel and using for the same purpose would be a better approach.
> First pass adoption of that subsystem revealed that there were two
> use-cases that current design didn't allow us to handle:
> 
> - Depending on the version i.MX SoC MAC address data stored in ROM
> would have different layout so as a possible solution to that I
> implemented "composite" driver(patch #3)
> 
> - On i.MX28, part of the MAC address is hard-coded in
> arch/arm/mach-mxs/mach-mxs.c and only a portion of it is read from
> ROM, this patch in combination with the aforementioned one should
> allow us to encode all needed info in DT.
> 
> Ideally, since all of the above is as applicable to Linux as it is to
> Barebox it would be good for BB not to invent its own custom 'nvmem'
> flavor, so hence me trying to start a conversation about adding this
> upstream.

If the only use-case is to store the MAC address, why not using the
local-mac-address property directly?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Andrey Smirnov]

>> >
>> > Patch itself looks simple. Before we review it further could you provide
>> > more details on the exact usecase or some background of this.
>>
>> The discussion on this topic originated on mailing list of Barebox
>> project(which borrows very heavily from Linux designs). Barebox
>> operates on two device tree blobs, one is used for its internal
>> initialization, whereas second one is passed to Linux kernel when
>> booting it. The problem I was trying to solve was to make possible to
>> specify in the first DT blob what data would be used for MAC address
>> fixup of the second DT blob(the one passed to Linux).
>>
>> My first approach was to implement a very limited DT code, however in
>> discussing it the consensus was that porting 'nvmem' subsystem from
>> the kernel and using for the same purpose would be a better approach.
>> First pass adoption of that subsystem revealed that there were two
>> use-cases that current design didn't allow us to handle:
>>
>> - Depending on the version i.MX SoC MAC address data stored in ROM
>> would have different layout so as a possible solution to that I
>> implemented "composite" driver(patch #3)
>>
>> - On i.MX28, part of the MAC address is hard-coded in
>> arch/arm/mach-mxs/mach-mxs.c and only a portion of it is read from
>> ROM, this patch in combination with the aforementioned one should
>> allow us to encode all needed info in DT.
>>
>> Ideally, since all of the above is as applicable to Linux as it is to
>> Barebox it would be good for BB not to invent its own custom 'nvmem'
>> flavor, so hence me trying to start a conversation about adding this
>> upstream.
>
> If the only use-case is to store the MAC address, why not using the
> local-mac-address property directly?

I don't think I understand what you mean, could you give me an example
of how I'd use local-mac-address property for that use case? AFAIK,
local-mac-address is just an array of bytes embedded into device tree,
how would it get populated with data from OTP memory of SoC?

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]

Hi,

On Mon, Mar 07, 2016 at 08:07:41PM -0800, Andrey Smirnov wrote:
> >> >
> >> > Patch itself looks simple. Before we review it further could you provide
> >> > more details on the exact usecase or some background of this.
> >>
> >> The discussion on this topic originated on mailing list of Barebox
> >> project(which borrows very heavily from Linux designs). Barebox
> >> operates on two device tree blobs, one is used for its internal
> >> initialization, whereas second one is passed to Linux kernel when
> >> booting it. The problem I was trying to solve was to make possible to
> >> specify in the first DT blob what data would be used for MAC address
> >> fixup of the second DT blob(the one passed to Linux).
> >>
> >> My first approach was to implement a very limited DT code, however in
> >> discussing it the consensus was that porting 'nvmem' subsystem from
> >> the kernel and using for the same purpose would be a better approach.
> >> First pass adoption of that subsystem revealed that there were two
> >> use-cases that current design didn't allow us to handle:
> >>
> >> - Depending on the version i.MX SoC MAC address data stored in ROM
> >> would have different layout so as a possible solution to that I
> >> implemented "composite" driver(patch #3)
> >>
> >> - On i.MX28, part of the MAC address is hard-coded in
> >> arch/arm/mach-mxs/mach-mxs.c and only a portion of it is read from
> >> ROM, this patch in combination with the aforementioned one should
> >> allow us to encode all needed info in DT.
> >>
> >> Ideally, since all of the above is as applicable to Linux as it is to
> >> Barebox it would be good for BB not to invent its own custom 'nvmem'
> >> flavor, so hence me trying to start a conversation about adding this
> >> upstream.
> >
> > If the only use-case is to store the MAC address, why not using the
> > local-mac-address property directly?
> 
> I don't think I understand what you mean, could you give me an example
> of how I'd use local-mac-address property for that use case? AFAIK,
> local-mac-address is just an array of bytes embedded into device tree,

Well, yeah, but the nvmem-blob is also just an array of bytes embedded
into the DT, right?

> how would it get populated with data from OTP memory of SoC?

In the bootloader, or Linux, read the OTP, patch the DT to add that
node, done.

It's already what U-Boot does here for example (the source of ethaddr
being dependent of the platform):
http://git.denx.de/?p=u-boot.git;a=blob;f=common/fdt_support.c#l468

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Andrey Smirnov]

>>
>> I don't think I understand what you mean, could you give me an example
>> of how I'd use local-mac-address property for that use case? AFAIK,
>> local-mac-address is just an array of bytes embedded into device tree,
>
> Well, yeah, but the nvmem-blob is also just an array of bytes embedded
> into the DT, right?

One is accessible via "nvmem" API and the other one isn't.

>
>> how would it get populated with data from OTP memory of SoC?
>
> In the bootloader, or Linux, read the OTP, patch the DT to add that
> node, done.

No, it's not really "done", because if you read my previous messages,
"read the OTP, patch the DT" is exactly the problem I am trying to
solve. The overall goal is to be able to read a certain "nvmem" cell
and patch DT with that data as MAC address, however in it's current
incarnation "nvmem" doesn't have provisions to make cells that are
just combination of other cells (patch #3) and to embed certain data
in DT and then access it as "nvmem" cell (patch #2)

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Trent Piepho]

On Tue, 2016-03-08 at 14:46 -0800, Andrey Smirnov wrote:
> >> I don't think I understand what you mean, could you give me an example
> >> of how I'd use local-mac-address property for that use case? AFAIK,
> >> local-mac-address is just an array of bytes embedded into device tree,
> >
> > Well, yeah, but the nvmem-blob is also just an array of bytes embedded
> > into the DT, right?
> 
> One is accessible via "nvmem" API and the other one isn't.
> 
> >
> >> how would it get populated with data from OTP memory of SoC?
> >
> > In the bootloader, or Linux, read the OTP, patch the DT to add that
> > node, done.
> 
> No, it's not really "done", because if you read my previous messages,
> "read the OTP, patch the DT" is exactly the problem I am trying to
> solve. The overall goal is to be able to read a certain "nvmem" cell
> and patch DT with that data as MAC address, however in it's current
> incarnation "nvmem" doesn't have provisions to make cells that are
> just combination of other cells (patch #3) and to embed certain data

So I did something that solved this a few years ago for another board
and embedded the data into the local-mac-address property.

I think maybe the problem isn't here isn't clear.  For some boards
(mxs), only part of a MAC address is stored in nvmem while the rest of
the address is fixed data.  Currently embedded in the kernel source,
though embedded in the device tree would clearly be better.  While
splitting the MAC into two locations like this is not, at least in my
opinion, the best design, it's burned into the one time programmable
memory so there's not much that can be done.

The kernel looks for 'mac-address' and then 'local-mac-address', with
the former having precedence.

So what I did was embed the fixed portion in 'local-mac-address', have
the bootloader extract the variable part from nvmem, and then combine
the two and place it into 'mac-address' for the kernel to use.

My DT binding that described the placement of data from nvmem into the
MAC used a permutation list instead of a series of ranges.  So a list
like [0 0 0 1 2 3] could be used to specify the first three bytes of the
mac address do not come from nvmem.  This also allows changing byte
order concisely.  But doesn't scale well to larger regions.

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On Tue, Mar 08, 2016 at 11:24:33PM +0000, Trent Piepho wrote:
> On Tue, 2016-03-08 at 14:46 -0800, Andrey Smirnov wrote:
> > >> I don't think I understand what you mean, could you give me an example
> > >> of how I'd use local-mac-address property for that use case? AFAIK,
> > >> local-mac-address is just an array of bytes embedded into device tree,
> > >
> > > Well, yeah, but the nvmem-blob is also just an array of bytes embedded
> > > into the DT, right?
> > 
> > One is accessible via "nvmem" API and the other one isn't.
> > 
> > >
> > >> how would it get populated with data from OTP memory of SoC?
> > >
> > > In the bootloader, or Linux, read the OTP, patch the DT to add that
> > > node, done.
> > 
> > No, it's not really "done", because if you read my previous messages,
> > "read the OTP, patch the DT" is exactly the problem I am trying to
> > solve. The overall goal is to be able to read a certain "nvmem" cell
> > and patch DT with that data as MAC address, however in it's current
> > incarnation "nvmem" doesn't have provisions to make cells that are
> > just combination of other cells (patch #3) and to embed certain data
> 
> So I did something that solved this a few years ago for another board
> and embedded the data into the local-mac-address property.
> 
> I think maybe the problem isn't here isn't clear.  For some boards
> (mxs), only part of a MAC address is stored in nvmem while the rest of
> the address is fixed data.  Currently embedded in the kernel source,
> though embedded in the device tree would clearly be better.  While
> splitting the MAC into two locations like this is not, at least in my
> opinion, the best design, it's burned into the one time programmable
> memory so there's not much that can be done.
> 
> The kernel looks for 'mac-address' and then 'local-mac-address', with
> the former having precedence.
> 
> So what I did was embed the fixed portion in 'local-mac-address', have
> the bootloader extract the variable part from nvmem, and then combine
> the two and place it into 'mac-address' for the kernel to use.

I guess another solution would be to deal with that at the driver
level. Read the 3 last bytes from the OTP, add the MAC prefix and give
that to the net framework without looking it up in the DT. The MAC
prefix could even be changed then through a kernel parameter, which is
not possible currently.

> My DT binding that described the placement of data from nvmem into the
> MAC used a permutation list instead of a series of ranges.  So a list
> like [0 0 0 1 2 3] could be used to specify the first three bytes of the
> mac address do not come from nvmem.  This also allows changing byte
> order concisely.  But doesn't scale well to larger regions.

Is your code / binding doc somewhere for reference ?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Trent Piepho]

On Wed, 2016-03-09 at 11:13 +0100, Maxime Ripard wrote:
> On Tue, Mar 08, 2016 at 11:24:33PM +0000, Trent Piepho wrote:
> > I think maybe the problem isn't here isn't clear.  For some boards
> > (mxs), only part of a MAC address is stored in nvmem while the rest of
> > the address is fixed data.  Currently embedded in the kernel source,
> > though embedded in the device tree would clearly be better.  While
> > splitting the MAC into two locations like this is not, at least in my
> > opinion, the best design, it's burned into the one time programmable
> > memory so there's not much that can be done.
> > 
> > The kernel looks for 'mac-address' and then 'local-mac-address', with
> > the former having precedence.
> > 
> > So what I did was embed the fixed portion in 'local-mac-address', have
> > the bootloader extract the variable part from nvmem, and then combine
> > the two and place it into 'mac-address' for the kernel to use.
> 
> I guess another solution would be to deal with that at the driver
> level. Read the 3 last bytes from the OTP, add the MAC prefix and give
> that to the net framework without looking it up in the DT. The MAC
> prefix could even be changed then through a kernel parameter, which is
> not possible currently.

How the data is stored in nvmem is specific to each board.  Some boards
use OTP, some use an external EEPROM.  The EEPROM could be on any of
multiple SPI or I2C buses and at different possible addresses.  The data
in OPT might be little-endian or big-endian or "written by someone who
doesn't understand byte order"-endian.

So the ethernet driver would need to deal with many different possible
board configurations.  And each ethernet driver would need to do that
all over again.

So the idea is to create a way that a single "DT binding for a MAC from
nvmem" driver could do the job for all boards and all ethernet drivers.
While there are many possibilities, a DT binding that covers all of them
doesn't need to be that complex.

> > My DT binding that described the placement of data from nvmem into the
> > MAC used a permutation list instead of a series of ranges.  So a list
> > like [0 0 0 1 2 3] could be used to specify the first three bytes of the
> > mac address do not come from nvmem.  This also allows changing byte
> > order concisely.  But doesn't scale well to larger regions.
> 
> Is your code / binding doc somewhere for reference ?

No, it was for a product that never upstreamed those changes :(.  It was
somewhat like the bindings that already exist in c6x/dscr.txt, for
ti,dscr-mac-fuse-regs.  I didn't support EEPROMs for that board, but
once extended to support that it would like something like:

&ethernet-device {
    mac-address   = [ab cd ef 00 00 00]; //already defined binding
    mac-nvmem     = <&otp 0x80 4>;  // these two are new bindings
    mac-nvmem-map = [0 0 0 1 2 3];
};

mac-nvmem = tuple of phandle, offset, length.  phandle is to a nvmem
device (OTP, eeprom, etc.).

mac-nvmem-map = list of bytes the length of a MAC address.  Each byte
indicates what byte from mac-nvmem (starting at 1, not 0) should go into
the corresponding position in the MAC.  0 means no byte from mac-nvmem
should be used for that position.

Thus [0 0 0 1 2 3] means the first three bytes of the mac do not come
from OTP and default to the existing value of mac-address.  The fourth
byte of the MAC should be the 1st byte from mac-nvmem, the fifth byte
shoud be the 2nd byte, and so on.

I believe this concisely handles all the possible cases of byte order,
EEPROMs, bytes in DT combined with bytes in OTP, etc. that I know of as
being used.

It doesn't attempt to be as powerful as Andrey's bindings.  E.g., it
doesn't scale to data much longer than a MAC address as the permutation
map list would become cumbersomely long.  It also doesn't support
combining data from more than two sources.

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Tue, Mar 08, 2016 at 02:46:52PM -0800, Andrey Smirnov wrote:
> >>
> >> I don't think I understand what you mean, could you give me an example
> >> of how I'd use local-mac-address property for that use case? AFAIK,
> >> local-mac-address is just an array of bytes embedded into device tree,
> >
> > Well, yeah, but the nvmem-blob is also just an array of bytes embedded
> > into the DT, right?
> 
> One is accessible via "nvmem" API and the other one isn't.

But there's no point in accessing it with nvmem in the first
place. You want to provide some information to the ethernet driver to
give it his mac address, that's what you want to do. And we already
have a property that does just that, and is supported by all the
ethernet drivers. Why would you want to add a special case to
something that is doing exactly the same thing?

> >> how would it get populated with data from OTP memory of SoC?
> >
> > In the bootloader, or Linux, read the OTP, patch the DT to add that
> > node, done.
> 
> No, it's not really "done", because if you read my previous messages,
> "read the OTP, patch the DT" is exactly the problem I am trying to
> solve. The overall goal is to be able to read a certain "nvmem" cell
> and patch DT with that data as MAC address,

But *why* do you need to store that using nvmem in the first place?
You really have two solutions here: make your ethernet driver read the
EEPROM using nvmem directly or do it in the bootloader and set
local-mac-address (note that you can do both).

> however in it's current incarnation "nvmem" doesn't have provisions
> to make cells that are just combination of other cells (patch #3)

This use case is valid.

> and to embed certain data in DT and then access it as "nvmem" cell
> (patch #2)

And this one is redundant.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Andrey Smirnov]

On Wed, Mar 9, 2016 at 1:58 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Mar 08, 2016 at 02:46:52PM -0800, Andrey Smirnov wrote:
>> >>
>> >> I don't think I understand what you mean, could you give me an example
>> >> of how I'd use local-mac-address property for that use case? AFAIK,
>> >> local-mac-address is just an array of bytes embedded into device tree,
>> >
>> > Well, yeah, but the nvmem-blob is also just an array of bytes embedded
>> > into the DT, right?
>>
>> One is accessible via "nvmem" API and the other one isn't.
>
> But there's no point in accessing it with nvmem in the first
> place. You want to provide some information to the ethernet driver to
> give it his mac address, that's what you want to do.

No, that's not what I want to do. I want to be able to deal with data
layout and composition of a "nvmem" cell at the data producer site and
not add custom hacks to the consumer code.

> And we already have a property that does just that, and is supported by all the
> ethernet drivers. Why would you want to add a special case to
> something that is doing exactly the same thing?

That's not what I am trying to add. The intent of "nvmem-blob" is to
be use as a part of a "nvmem-composite" cell in order to be able to
inject constants. It has nothing to do with storing complete
MAC-addresses, the fact that it can be used as a complete replacement
of "local-mac-address" is just a side-effect.

Sidenote: now that we are on the subject of using this code as
equivalent of "local-mac-address", I'd argue that if it were possible
to ignore backwards compatibility, removing "local-mac-address" from
all the drivers and .dts's and replacing it with something to the
effect of "nvmem-blob" would be a better solution since you'd be able
to handle the cases of MAC address being a part of DT and being stored
in NVM using exactly the same code-path.

>
>> >> how would it get populated with data from OTP memory of SoC?
>> >
>> > In the bootloader, or Linux, read the OTP, patch the DT to add that
>> > node, done.
>>
>> No, it's not really "done", because if you read my previous messages,
>> "read the OTP, patch the DT" is exactly the problem I am trying to
>> solve. The overall goal is to be able to read a certain "nvmem" cell
>> and patch DT with that data as MAC address,
>
> But *why* do you need to store that using nvmem in the first place?

Because I want to combine that data with other nvmem blobs using
"nvmem-composite"

> You really have two solutions here: make your ethernet driver read the
> EEPROM using nvmem directly or do it in the bootloader and set
> local-mac-address (note that you can do both).

Which would lead to per-board/per-SoC custom code. I've had that kind
of solution before I started this discussion. I am hoping that we all
can come up with more elegant solution and that's why I started this
thread.

>
>> however in it's current incarnation "nvmem" doesn't have provisions
>> to make cells that are just combination of other cells (patch #3)
>
> This use case is valid.
>
>> and to embed certain data in DT and then access it as "nvmem" cell
>> (patch #2)
>
> And this one is redundant.

How is it redundant? What kernel code allows one to create a constant
"nvmem" cell?

And why do you think #3 valid? Your solution for #2, namely:

> make your ethernet driver read the
> EEPROM using nvmem directly or do it in the bootloader and set
> local-mac-address (note that you can do both)

applies in the case of #3 as well. I can read and shuffle the data in
either consumer side (in this particular case the Ethernet driver) or
in the bootloader.

Re: [RESEND RFC 2/3] nvmem: Add 'nvmem-blob' driver

[Sascha Hauer]

On Tue, Mar 08, 2016 at 11:28:21PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Mar 07, 2016 at 08:07:41PM -0800, Andrey Smirnov wrote:
> > >> >
> > >> > Patch itself looks simple. Before we review it further could you provide
> > >> > more details on the exact usecase or some background of this.
> > >>
> > >> The discussion on this topic originated on mailing list of Barebox
> > >> project(which borrows very heavily from Linux designs). Barebox
> > >> operates on two device tree blobs, one is used for its internal
> > >> initialization, whereas second one is passed to Linux kernel when
> > >> booting it. The problem I was trying to solve was to make possible to
> > >> specify in the first DT blob what data would be used for MAC address
> > >> fixup of the second DT blob(the one passed to Linux).
> > >>
> > >> My first approach was to implement a very limited DT code, however in
> > >> discussing it the consensus was that porting 'nvmem' subsystem from
> > >> the kernel and using for the same purpose would be a better approach.
> > >> First pass adoption of that subsystem revealed that there were two
> > >> use-cases that current design didn't allow us to handle:
> > >>
> > >> - Depending on the version i.MX SoC MAC address data stored in ROM
> > >> would have different layout so as a possible solution to that I
> > >> implemented "composite" driver(patch #3)
> > >>
> > >> - On i.MX28, part of the MAC address is hard-coded in
> > >> arch/arm/mach-mxs/mach-mxs.c and only a portion of it is read from
> > >> ROM, this patch in combination with the aforementioned one should
> > >> allow us to encode all needed info in DT.
> > >>
> > >> Ideally, since all of the above is as applicable to Linux as it is to
> > >> Barebox it would be good for BB not to invent its own custom 'nvmem'
> > >> flavor, so hence me trying to start a conversation about adding this
> > >> upstream.
> > >
> > > If the only use-case is to store the MAC address, why not using the
> > > local-mac-address property directly?
> > 
> > I don't think I understand what you mean, could you give me an example
> > of how I'd use local-mac-address property for that use case? AFAIK,
> > local-mac-address is just an array of bytes embedded into device tree,
> 
> Well, yeah, but the nvmem-blob is also just an array of bytes embedded
> into the DT, right?
> 
> > how would it get populated with data from OTP memory of SoC?
> 
> In the bootloader, or Linux, read the OTP, patch the DT to add that
> node, done.

This series is a try to put the information how the MAC address can be
found (might be board specific) into the DT to not have board specific
hacks in the kernel/bootloader.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Andrey Smirnov]

Add 'nvmem-composite' driver which allows to combine multiple chunks of
various NVMEM cells into a single continuous NVMEM device.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 .../devicetree/bindings/nvmem/composite.txt        |  44 ++++
 drivers/nvmem/Makefile                             |   1 +
 drivers/nvmem/composite.c                          | 225 +++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/composite.txt
 create mode 100644 drivers/nvmem/composite.c

diff --git a/Documentation/devicetree/bindings/nvmem/composite.txt b/Documentation/devicetree/bindings/nvmem/composite.txt
new file mode 100644
index 0000000..e24cf4b
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/composite.txt
@@ -0,0 +1,44 @@
+= NVMEM cell compositor =
+
+This binding is designed to provide a way for a developer to combine
+portions of other NVMEM cell and acces that data as a signle NVMEM
+cell using NVMEM consumer API.
+
+Required properties:
+- compatible: should be "nvmem-composite"
+- layout: specifies which sources comprise data in nvmem device
+  	  format is "<nvmem-cell-phandle offset size>"
+
+= Data cells =
+Are child nodes of nvmem-composite, bindings of which as described in
+bindings/nvmem/nvmem.txt
+
+Example:
+
+	composite-nvmem {
+		compatible = "nvmem-composite";
+		layout = <&another_cell_a 0 2
+ 			  &another_cell_b 0 1
+			  &another_cell_c 3 2>;
+
+		cell1: cell@0 {
+			reg = <0 5>;
+		};
+	}
+
+the result of reading variable cell1 would be:
+
+[a[0] a[1] b[0] c[3] c[4]],
+
+where a[i], b[i], c[i], represnet i-th bytes of NVMEM cells
+another_cell_a, another_cell_b and another_cell_c respectively
+
+= Data consumers =
+Are device nodes which consume nvmem data cells.
+
+Example:
+
+	a-node {
+		nvmem-cells = <&cell1>;
+		nvmem-cell-names = "some-data";
+	};
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 1a1adba..49c0eca 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -19,3 +19,4 @@ nvmem_sunxi_sid-y		:= sunxi_sid.o
 obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
 nvmem-vf610-ocotp-y		:= vf610-ocotp.o
 obj-y += blob.o
+obj-y += composite.o
diff --git a/drivers/nvmem/composite.c b/drivers/nvmem/composite.c
new file mode 100644
index 0000000..cf01590
--- /dev/null
+++ b/drivers/nvmem/composite.c
@@ -0,0 +1,225 @@
+#define DEBUG 1
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct nvmem_composite {
+	struct device *dev;
+	struct list_head layout;
+	size_t layout_size;
+};
+
+struct nvmem_composite_item {
+	struct nvmem_cell *cell;
+	unsigned int idx, start, end, size;
+	struct list_head node;
+};
+
+static struct nvmem_composite_item *
+nvmem_composite_find_first(struct nvmem_composite *ncomp,
+			   unsigned int offset)
+{
+	struct nvmem_composite_item *first;
+	list_for_each_entry(first, &ncomp->layout, node) {
+		/* 
+		 * Skip all of the irrelevant items that end before our offset
+		 */
+		if (first->end > offset)
+			return first;
+	}
+
+	return NULL;
+}
+
+static int nvmem_composite_read(void *context,
+				const void *reg, size_t reg_size,
+				void *val, size_t val_size)
+{
+	struct nvmem_composite *ncomp = context;
+	const unsigned int offset = *(u32 *)reg;
+	void *dst = val;
+	size_t residue = val_size;
+	struct nvmem_composite_item *item, *first;
+	uint8_t *data;
+	unsigned int size, chunk, ii;
+
+
+	first = item = nvmem_composite_find_first(ncomp, offset);
+	if (!first) {
+		dev_dbg(ncomp->dev, "Invalid offset\n");
+		return -EINVAL;
+	}
+
+	list_for_each_entry_from(item, &ncomp->layout, node) {
+		/* 
+		 * If our first read is not located on item boundary
+		 * we have to introduce artificial offset
+		 */
+		ii = (item == first) ? offset - first->start : 0;
+
+		data = nvmem_cell_read(item->cell, &size);
+		if (IS_ERR(data)) {
+			dev_dbg(ncomp->dev, "Failed to read nvmem cell\n");
+			return PTR_ERR(data);
+		}
+
+		chunk = min(residue, item->size - ii);
+		memcpy(dst, &data[item->idx + ii], chunk);
+		kfree(data);
+
+		dst += chunk;
+		residue -= chunk;
+	}
+
+	return (residue) ? -EINVAL : 0;
+}
+
+static int nvmem_composite_write(void *context, const void *data,
+				 size_t count)
+{
+	return -ENOTSUPP;
+}
+
+static const struct regmap_bus nvmem_composite_regmap_bus = {
+	.write = nvmem_composite_write,
+	.read  = nvmem_composite_read,
+};
+
+static int nvmem_composite_validate_dt(struct device_node *np)
+{
+	/* FIXME */
+	return 0;
+}
+
+static int nvmem_composite_probe(struct platform_device *pdev)
+{
+	int i, ret;
+	unsigned int start = 0;
+	unsigned int len;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct nvmem_device *nvmem;
+	struct nvmem_composite *ncomp;
+	struct regmap *map;
+	struct nvmem_composite_item *item;
+	struct nvmem_config  nv_cnf = {0};
+	struct regmap_config rm_cnf = {0};
+	const __be32 *addr;
+	unsigned int item_count;
+
+	ret = nvmem_composite_validate_dt(np);
+	if (ret < 0) {
+		dev_dbg(dev, "Device validation failed\n");
+		return ret;
+	}
+
+	ncomp = devm_kzalloc(dev, sizeof(*ncomp), GFP_KERNEL);
+	INIT_LIST_HEAD(&ncomp->layout);
+	ncomp->dev = dev;
+	
+	addr = of_get_property(np, "layout", &len);
+	item_count = len / (3 * sizeof(__be32));
+
+	for (i = 0; i < item_count; i++) {
+		struct device_node *cell_np;
+		uint32_t phandle;
+
+		item = devm_kzalloc(dev, sizeof(*item), GFP_KERNEL);
+
+		phandle = be32_to_cpup(addr++);
+		cell_np = of_find_node_by_phandle(phandle);
+		if (!cell_np) {
+			dev_dbg(dev,
+				"Couldn't find nvmem cell by its phandle\n");
+			return -ENOENT;
+		}
+
+		item->cell = of_nvmem_cell_from_device_node(cell_np);
+		if (IS_ERR(item->cell)) {
+			dev_dbg(dev,
+				"Failed to instantiate nvmem cell from "
+				"a device tree node\n");
+			ret = PTR_ERR(item->cell);
+			goto unwind;
+		}
+
+		item->start = start;
+		item->idx = be32_to_cpup(addr++);
+		item->size = be32_to_cpup(addr++);
+		item->end  = item->size - 1;
+		ncomp->layout_size += item->size;
+		start += item->size;
+
+		list_add_tail(&item->node, &ncomp->layout);
+	}
+
+	rm_cnf.reg_bits = 32;
+	rm_cnf.val_bits = 8;
+	rm_cnf.reg_stride = 1;
+	rm_cnf.name = "nvmem-composite";
+	rm_cnf.max_register = ncomp->layout_size - 1;
+
+	map = devm_regmap_init(dev,
+			       &nvmem_composite_regmap_bus,
+			       ncomp,
+			       &rm_cnf);
+	if (IS_ERR(map)) {
+		dev_dbg(dev, "Failed to initilize regmap\n");
+		return PTR_ERR(map);
+	}
+
+	nv_cnf.name = "nvmem-composite";
+	nv_cnf.read_only = true;
+	nv_cnf.dev = dev;
+
+	nvmem = nvmem_register(&nv_cnf);
+	if (IS_ERR(nvmem)) {
+		dev_dbg(dev, "Failed to register 'nvmem' device\n");
+		return PTR_ERR(nvmem);
+	}
+
+	platform_set_drvdata(pdev, nvmem);
+	return 0;
+unwind:
+	/* FIXME  */
+	return ret;
+}
+
+static int nvmem_composite_remove(struct platform_device *pdev)
+{
+	struct nvmem_device *nvmem = platform_get_drvdata(pdev);
+
+	/* 
+	   FIXME free allocated nvmem cells
+	 */
+	return nvmem_unregister(nvmem);
+}
+
+
+static const struct of_device_id nvmem_composite_dt_ids[] = {
+	{ .compatible = "nvmem-composite", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, nvmem_composite_dt_ids);
+
+static struct platform_driver nvmem_composite_driver = {
+	.probe	= nvmem_composite_probe,
+	.remove	= nvmem_composite_remove,
+	.driver = {
+		.name	= "nvmem-composite",
+		.of_match_table = nvmem_composite_dt_ids,
+	},
+};
+module_platform_driver(nvmem_composite_driver);
+
+MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
+MODULE_DESCRIPTION("FIXME");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Srinivas Kandagatla]

On 01/03/16 16:59, Andrey Smirnov wrote:
> Add 'nvmem-composite' driver which allows to combine multiple chunks of
> various NVMEM cells into a single continuous NVMEM device.

My plan on this feature was add support inside the nvmem_cell_get 
itself, this makes the nvmem bindings more inline with bindings like 
pinctrl.
Also I still want to keep nvmem simple as it can.

DT would look something like this.

nvmem-provider-a {
	cell_a {
		reg = <0 2>;
	};
};

nvmem-provider-b {
	cell_b: cell_c {
		reg = <0 1>;
	};
};

nvmem-provider-c {
	cell_c: cell_c {
		reg = <3 2>;
	}
};

a-node {
	nvmem-cells = <&cell_a &cell_b &cell_c>
	nvmem-cell-names = "some-data";
};


thanks,
srini

>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   .../devicetree/bindings/nvmem/composite.txt        |  44 ++++
>   drivers/nvmem/Makefile                             |   1 +
>   drivers/nvmem/composite.c                          | 225 +++++++++++++++++++++
>   3 files changed, 270 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/nvmem/composite.txt
>   create mode 100644 drivers/nvmem/composite.c
>
> diff --git a/Documentation/devicetree/bindings/nvmem/composite.txt b/Documentation/devicetree/bindings/nvmem/composite.txt
> new file mode 100644
> index 0000000..e24cf4b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/composite.txt
> @@ -0,0 +1,44 @@
> += NVMEM cell compositor =
> +
> +This binding is designed to provide a way for a developer to combine
> +portions of other NVMEM cell and acces that data as a signle NVMEM
> +cell using NVMEM consumer API.
> +
> +Required properties:
> +- compatible: should be "nvmem-composite"
> +- layout: specifies which sources comprise data in nvmem device
> +  	  format is "<nvmem-cell-phandle offset size>"
> +
> += Data cells =
> +Are child nodes of nvmem-composite, bindings of which as described in
> +bindings/nvmem/nvmem.txt
> +
> +Example:
> +
> +	composite-nvmem {
> +		compatible = "nvmem-composite";
> +		layout = <&another_cell_a 0 2
> + 			  &another_cell_b 0 1
> +			  &another_cell_c 3 2>;
> +
> +		cell1: cell@0 {
> +			reg = <0 5>;
> +		};
> +	}
> +
> +the result of reading variable cell1 would be:
> +
> +[a[0] a[1] b[0] c[3] c[4]],
> +
> +where a[i], b[i], c[i], represnet i-th bytes of NVMEM cells
> +another_cell_a, another_cell_b and another_cell_c respectively
> +
> += Data consumers =
> +Are device nodes which consume nvmem data cells.
> +
> +Example:
> +
> +	a-node {
> +		nvmem-cells = <&cell1>;
> +		nvmem-cell-names = "some-data";
> +	};
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 1a1adba..49c0eca 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -19,3 +19,4 @@ nvmem_sunxi_sid-y		:= sunxi_sid.o
>   obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
>   nvmem-vf610-ocotp-y		:= vf610-ocotp.o
>   obj-y += blob.o
> +obj-y += composite.o
> diff --git a/drivers/nvmem/composite.c b/drivers/nvmem/composite.c
> new file mode 100644
> index 0000000..cf01590
> --- /dev/null
> +++ b/drivers/nvmem/composite.c
> @@ -0,0 +1,225 @@
> +#define DEBUG 1
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +struct nvmem_composite {
> +	struct device *dev;
> +	struct list_head layout;
> +	size_t layout_size;
> +};
> +
> +struct nvmem_composite_item {
> +	struct nvmem_cell *cell;
> +	unsigned int idx, start, end, size;
> +	struct list_head node;
> +};
> +
> +static struct nvmem_composite_item *
> +nvmem_composite_find_first(struct nvmem_composite *ncomp,
> +			   unsigned int offset)
> +{
> +	struct nvmem_composite_item *first;
> +	list_for_each_entry(first, &ncomp->layout, node) {
> +		/*
> +		 * Skip all of the irrelevant items that end before our offset
> +		 */
> +		if (first->end > offset)
> +			return first;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int nvmem_composite_read(void *context,
> +				const void *reg, size_t reg_size,
> +				void *val, size_t val_size)
> +{
> +	struct nvmem_composite *ncomp = context;
> +	const unsigned int offset = *(u32 *)reg;
> +	void *dst = val;
> +	size_t residue = val_size;
> +	struct nvmem_composite_item *item, *first;
> +	uint8_t *data;
> +	unsigned int size, chunk, ii;
> +
> +
> +	first = item = nvmem_composite_find_first(ncomp, offset);
> +	if (!first) {
> +		dev_dbg(ncomp->dev, "Invalid offset\n");
> +		return -EINVAL;
> +	}
> +
> +	list_for_each_entry_from(item, &ncomp->layout, node) {
> +		/*
> +		 * If our first read is not located on item boundary
> +		 * we have to introduce artificial offset
> +		 */
> +		ii = (item == first) ? offset - first->start : 0;
> +
> +		data = nvmem_cell_read(item->cell, &size);
> +		if (IS_ERR(data)) {
> +			dev_dbg(ncomp->dev, "Failed to read nvmem cell\n");
> +			return PTR_ERR(data);
> +		}
> +
> +		chunk = min(residue, item->size - ii);
> +		memcpy(dst, &data[item->idx + ii], chunk);
> +		kfree(data);
> +
> +		dst += chunk;
> +		residue -= chunk;
> +	}
> +
> +	return (residue) ? -EINVAL : 0;
> +}
> +
> +static int nvmem_composite_write(void *context, const void *data,
> +				 size_t count)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static const struct regmap_bus nvmem_composite_regmap_bus = {
> +	.write = nvmem_composite_write,
> +	.read  = nvmem_composite_read,
> +};
> +
> +static int nvmem_composite_validate_dt(struct device_node *np)
> +{
> +	/* FIXME */
> +	return 0;
> +}
> +
> +static int nvmem_composite_probe(struct platform_device *pdev)
> +{
> +	int i, ret;
> +	unsigned int start = 0;
> +	unsigned int len;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct nvmem_device *nvmem;
> +	struct nvmem_composite *ncomp;
> +	struct regmap *map;
> +	struct nvmem_composite_item *item;
> +	struct nvmem_config  nv_cnf = {0};
> +	struct regmap_config rm_cnf = {0};
> +	const __be32 *addr;
> +	unsigned int item_count;
> +
> +	ret = nvmem_composite_validate_dt(np);
> +	if (ret < 0) {
> +		dev_dbg(dev, "Device validation failed\n");
> +		return ret;
> +	}
> +
> +	ncomp = devm_kzalloc(dev, sizeof(*ncomp), GFP_KERNEL);
> +	INIT_LIST_HEAD(&ncomp->layout);
> +	ncomp->dev = dev;
> +	
> +	addr = of_get_property(np, "layout", &len);
> +	item_count = len / (3 * sizeof(__be32));
> +
> +	for (i = 0; i < item_count; i++) {
> +		struct device_node *cell_np;
> +		uint32_t phandle;
> +
> +		item = devm_kzalloc(dev, sizeof(*item), GFP_KERNEL);
> +
> +		phandle = be32_to_cpup(addr++);
> +		cell_np = of_find_node_by_phandle(phandle);
> +		if (!cell_np) {
> +			dev_dbg(dev,
> +				"Couldn't find nvmem cell by its phandle\n");
> +			return -ENOENT;
> +		}
> +
> +		item->cell = of_nvmem_cell_from_device_node(cell_np);
> +		if (IS_ERR(item->cell)) {
> +			dev_dbg(dev,
> +				"Failed to instantiate nvmem cell from "
> +				"a device tree node\n");
> +			ret = PTR_ERR(item->cell);
> +			goto unwind;
> +		}
> +
> +		item->start = start;
> +		item->idx = be32_to_cpup(addr++);
> +		item->size = be32_to_cpup(addr++);
> +		item->end  = item->size - 1;
> +		ncomp->layout_size += item->size;
> +		start += item->size;
> +
> +		list_add_tail(&item->node, &ncomp->layout);
> +	}
> +
> +	rm_cnf.reg_bits = 32;
> +	rm_cnf.val_bits = 8;
> +	rm_cnf.reg_stride = 1;
> +	rm_cnf.name = "nvmem-composite";
> +	rm_cnf.max_register = ncomp->layout_size - 1;
> +
> +	map = devm_regmap_init(dev,
> +			       &nvmem_composite_regmap_bus,
> +			       ncomp,
> +			       &rm_cnf);
> +	if (IS_ERR(map)) {
> +		dev_dbg(dev, "Failed to initilize regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	nv_cnf.name = "nvmem-composite";
> +	nv_cnf.read_only = true;
> +	nv_cnf.dev = dev;
> +
> +	nvmem = nvmem_register(&nv_cnf);
> +	if (IS_ERR(nvmem)) {
> +		dev_dbg(dev, "Failed to register 'nvmem' device\n");
> +		return PTR_ERR(nvmem);
> +	}
> +
> +	platform_set_drvdata(pdev, nvmem);
> +	return 0;
> +unwind:
> +	/* FIXME  */
> +	return ret;
> +}
> +
> +static int nvmem_composite_remove(struct platform_device *pdev)
> +{
> +	struct nvmem_device *nvmem = platform_get_drvdata(pdev);
> +
> +	/*
> +	   FIXME free allocated nvmem cells
> +	 */
> +	return nvmem_unregister(nvmem);
> +}
> +
> +
> +static const struct of_device_id nvmem_composite_dt_ids[] = {
> +	{ .compatible = "nvmem-composite", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, nvmem_composite_dt_ids);
> +
> +static struct platform_driver nvmem_composite_driver = {
> +	.probe	= nvmem_composite_probe,
> +	.remove	= nvmem_composite_remove,
> +	.driver = {
> +		.name	= "nvmem-composite",
> +		.of_match_table = nvmem_composite_dt_ids,
> +	},
> +};
> +module_platform_driver(nvmem_composite_driver);
> +
> +MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
> +MODULE_DESCRIPTION("FIXME");
> +MODULE_LICENSE("GPL v2");
>

Re: [RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Andrey Smirnov]

On Wed, Mar 2, 2016 at 5:59 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 01/03/16 16:59, Andrey Smirnov wrote:
>>
>> Add 'nvmem-composite' driver which allows to combine multiple chunks of
>> various NVMEM cells into a single continuous NVMEM device.
>
>
> My plan on this feature was add support inside the nvmem_cell_get itself,
> this makes the nvmem bindings more inline with bindings like pinctrl.
> Also I still want to keep nvmem simple as it can.

That's perfectly fine with me, I can change my patch to do that. My
thinking on implementing it as a standalone module was that this seems
to be a very niche functionality which not a lot of people would use,
so the code would be a part of your kernel only if you directly use
this feature.

>
> DT would look something like this.
>
> nvmem-provider-a {
>         cell_a {
>                 reg = <0 2>;
>         };
> };
>
> nvmem-provider-b {
>         cell_b: cell_c {
>                 reg = <0 1>;
>         };
> };
>
> nvmem-provider-c {
>         cell_c: cell_c {
>                 reg = <3 2>;
>         }
> };
>
> a-node {
>         nvmem-cells = <&cell_a &cell_b &cell_c>
>         nvmem-cell-names = "some-data";
> };
>

It's not very clear to me, possibly to my DT ignorance, how this would
handle the case of multiple variables. Say we have

a-node {
         nvmem-cells = <&cell_a &cell_b>, ????;
         nvmem-cell-names = "some-data", "more-data";
};

and I want "more-data" to reference only one phandle, how would this be handled?

Another minor nitpick about his is that if one's goal is to just
byteswap already defined nvmem_cell:
  - N, where N is the length of the variable, new nvmem cells would
have to be defined
  - All of them would have to use absolute address within the nvmem
provided, instead of referencing a byte relative to the position of
pre-defined cell

Thanks,
Andrey

Re: [RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Srinivas Kandagatla]

On 02/03/16 18:33, Andrey Smirnov wrote:
> On Wed, Mar 2, 2016 at 5:59 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> On 01/03/16 16:59, Andrey Smirnov wrote:
>>>
>>> Add 'nvmem-composite' driver which allows to combine multiple chunks of
>>> various NVMEM cells into a single continuous NVMEM device.
>>
>>
>> My plan on this feature was add support inside the nvmem_cell_get itself,
>> this makes the nvmem bindings more inline with bindings like pinctrl.
>> Also I still want to keep nvmem simple as it can.
>
> That's perfectly fine with me, I can change my patch to do that. My
> thinking on implementing it as a standalone module was that this seems
> to be a very niche functionality which not a lot of people would use,
> so the code would be a part of your kernel only if you directly use
> this feature.
>
>>
>> DT would look something like this.
>>
>> nvmem-provider-a {
>>          cell_a {
>>                  reg = <0 2>;
>>          };
>> };
>>
>> nvmem-provider-b {
>>          cell_b: cell_c {
>>                  reg = <0 1>;
>>          };
>> };
>>
>> nvmem-provider-c {
>>          cell_c: cell_c {
>>                  reg = <3 2>;
>>          }
>> };
>>
>> a-node {
>>          nvmem-cells = <&cell_a &cell_b &cell_c>
>>          nvmem-cell-names = "some-data";
>> };
>>
>
> It's not very clear to me, possibly to my DT ignorance, how this would
> handle the case of multiple variables. Say we have
>
> a-node {
>           nvmem-cells = <&cell_a &cell_b>, ????;
>           nvmem-cell-names = "some-data", "more-data";
> };
>
Should have replied you long back :-)
> and I want "more-data" to reference only one phandle, how would this be handled?
>
yes this would fail.

The device tree compiler would concatenate all the cells and we have no 
means to know where did "more-data" starts.

sounds like composite driver is the way forward.


--srini
> Another minor nitpick about his is that if one's goal is to just
> byteswap already defined nvmem_cell:
>    - N, where N is the length of the variable, new nvmem cells would
> have to be defined
>    - All of them would have to use absolute address within the nvmem
> provided, instead of referencing a byte relative to the position of
> pre-defined cell
>
> Thanks,
> Andrey
>

Re: [RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Andrey Smirnov]

>> a-node {
>>           nvmem-cells = <&cell_a &cell_b>, ????;
>>           nvmem-cell-names = "some-data", "more-data";
>> };
>>
> Should have replied you long back :-)

No worries :-)

>>
>> and I want "more-data" to reference only one phandle, how would this be
>> handled?
>>
> yes this would fail.
>
> The device tree compiler would concatenate all the cells and we have no
> means to know where did "more-data" starts.
>
> sounds like composite driver is the way forward.

What's your preference on "separate driver" vs. "part of the NVMEM
framework"? Should I keep it as a separate driver or try to fold it
into NVMEM core?

Thanks,
Andrey

Re: [RESEND RFC 3/3] nvmem: Add 'nvmem-composite' driver

[Srinivas Kandagatla]

On 21/03/16 16:12, Andrey Smirnov wrote:
>>> a-node {
>>>            nvmem-cells = <&cell_a &cell_b>, ????;
>>>            nvmem-cell-names = "some-data", "more-data";
>>> };
>>>
>> Should have replied you long back :-)
>
> No worries :-)
>
>>>
>>> and I want "more-data" to reference only one phandle, how would this be
>>> handled?
>>>
>> yes this would fail.
>>
>> The device tree compiler would concatenate all the cells and we have no
>> means to know where did "more-data" starts.
>>
>> sounds like composite driver is the way forward.
>
> What's your preference on "separate driver" vs. "part of the NVMEM
> framework"? Should I keep it as a separate driver or try to fold it
> into NVMEM core?

It does not make sense to have this driver as a separate nvmem provider, 
can you fold it in the nvmem core itself for now.


Thanks,
srini
>
> Thanks,
> Andrey
>