[PATCH 0/6] nvmem: add "cell-type" property to support mac-address

[PATCH 0/6] nvmem: add "cell-type" property to support mac-address

[Joakim Zhang]

This patch set adds "cell-type" property to parse mac address, take i.MX
as an example, which need reverse byte for mac address.

Joakim Zhang (2):
  arm64: dts: imx8mm: add "cell-type" property for mac-address
  arm64: dts: imx8m: remove unused "nvmem_macaddr_swap" property for FEC

Srinivas Kandagatla (4):
  dt-bindings: nvmem: add cell-type to nvmem cells
  nvmem: core: parse nvmem cell-type from device tree
  nvmem: core: add nvmem cell post processing callback
  nvmem: imx-ocotp: add support for post porcessing.

 .../devicetree/bindings/nvmem/nvmem.yaml      | 11 +++++++
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  3 +-
 arch/arm64/boot/dts/freescale/imx8mn.dtsi     |  3 +-
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     | 10 ++++++-
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  3 +-
 drivers/nvmem/core.c                          | 12 ++++++++
 drivers/nvmem/imx-ocotp.c                     | 30 +++++++++++++++++++
 include/dt-bindings/nvmem/nvmem.h             |  8 +++++
 include/linux/nvmem-provider.h                |  5 ++++
 9 files changed, 81 insertions(+), 4 deletions(-)
 create mode 100644 include/dt-bindings/nvmem/nvmem.h

-- 
2.17.1

[PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Joakim Zhang]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Some of the nvmem providers encode data for certain type of nvmem cell,
example mac-address is stored in ascii or with delimiter or in reverse order.

This is much specific to vendor, so having a cell-type would allow nvmem
provider drivers to post-process this before using it.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
 include/dt-bindings/nvmem/nvmem.h                  |  8 ++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/nvmem/nvmem.h

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index b8dc3d2b6e92..8cf6c7e72b0a 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -60,6 +60,11 @@ patternProperties:
             - minimum: 1
               description:
                 Size in bit within the address range specified by reg.
+      cell-type:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        maxItems: 1
+        description:
+          Type of nvmem, Use defines in dt-bindings/nvmem/nvmem.h.
 
     required:
       - reg
@@ -69,6 +74,7 @@ additionalProperties: true
 examples:
   - |
       #include <dt-bindings/gpio/gpio.h>
+      #include <dt-bindings/nvmem/nvmem.h>
 
       qfprom: eeprom@700000 {
           #address-cells = <1>;
@@ -98,6 +104,11 @@ examples:
               reg = <0xc 0x1>;
               bits = <2 3>;
           };
+
+          mac_addr: mac-addr@90{
+              reg = <0x90 0x6>;
+              cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
+          };
       };
 
 ...
diff --git a/include/dt-bindings/nvmem/nvmem.h b/include/dt-bindings/nvmem/nvmem.h
new file mode 100644
index 000000000000..eed0478f6bfd
--- /dev/null
+++ b/include/dt-bindings/nvmem/nvmem.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DT_NVMMEM_H
+#define __DT_NVMMEM_H
+
+#define NVMEM_CELL_TYPE_UNKNOWN		0
+#define NVMEM_CELL_TYPE_MAC_ADDRESS	1
+
+#endif /* __DT_NVMMEM_H */
-- 
2.17.1

[PATCH 2/6] nvmem: core: parse nvmem cell-type from device tree

[Joakim Zhang]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

get nvmem cell-type from device tree

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/nvmem/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 3d87fadaa160..23c08dbaf45e 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -52,6 +52,7 @@ struct nvmem_cell {
 	int			bytes;
 	int			bit_offset;
 	int			nbits;
+	u32			type;
 	struct device_node	*np;
 	struct nvmem_device	*nvmem;
 	struct list_head	node;
@@ -726,6 +727,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 			return -EINVAL;
 		}
 
+		of_property_read_u32(child, "cell-type", &cell->type);
+
 		cell->np = of_node_get(child);
 		nvmem_cell_add(cell);
 	}
-- 
2.17.1

[PATCH 3/6] nvmem: core: add nvmem cell post processing callback

[Joakim Zhang]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Some NVMEM providers have certain nvmem cells encoded, which requires
post processing before actually using it.

For example mac-address is stored in either in ascii or delimited or reverse-order.

Having a post-process callback hook to provider drivers would enable them to
do this vendor specific post processing before nvmem consumers see it.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/nvmem/core.c           | 9 +++++++++
 include/linux/nvmem-provider.h | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 23c08dbaf45e..4f81a3adf081 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -38,6 +38,7 @@ struct nvmem_device {
 	unsigned int		nkeepout;
 	nvmem_reg_read_t	reg_read;
 	nvmem_reg_write_t	reg_write;
+	nvmem_cell_post_process_t cell_post_process;
 	struct gpio_desc	*wp_gpio;
 	void *priv;
 };
@@ -797,6 +798,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->type = config->type;
 	nvmem->reg_read = config->reg_read;
 	nvmem->reg_write = config->reg_write;
+	nvmem->cell_post_process = config->cell_post_process;
 	nvmem->keepout = config->keepout;
 	nvmem->nkeepout = config->nkeepout;
 	if (config->of_node)
@@ -1404,6 +1406,13 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 	if (cell->bit_offset || cell->nbits)
 		nvmem_shift_read_buffer_in_place(cell, buf);
 
+	if (nvmem->cell_post_process) {
+		rc = nvmem->cell_post_process(nvmem->priv, cell->type,
+					      cell->offset, buf, cell->bytes);
+		if (rc)
+			return rc;
+	}
+
 	if (len)
 		*len = cell->bytes;
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 104505e9028f..d980c79f9605 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -19,6 +19,9 @@ 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);
+/* used for vendor specific post processing of cell data */
+typedef int (*nvmem_cell_post_process_t)(void *priv, int type, unsigned int offset,
+					  void *buf, size_t bytes);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
@@ -62,6 +65,7 @@ struct nvmem_keepout {
  * @no_of_node:	Device should not use the parent's of_node even if it's !NULL.
  * @reg_read:	Callback to read data.
  * @reg_write:	Callback to write data.
+ * @cell_read_callback: Callback for vendor specific post processing of cell data
  * @size:	Device size.
  * @word_size:	Minimum read/write access granularity.
  * @stride:	Minimum read/write access stride.
@@ -92,6 +96,7 @@ struct nvmem_config {
 	bool			no_of_node;
 	nvmem_reg_read_t	reg_read;
 	nvmem_reg_write_t	reg_write;
+	nvmem_cell_post_process_t cell_post_process;
 	int	size;
 	int	word_size;
 	int	stride;
-- 
2.17.1

[PATCH 4/6] nvmem: imx-ocotp: add support for post porcessing.

[Joakim Zhang]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Add .cell_post_process callback for imx-ocotp to deal with MAC address,
since MAC address need to be reversed byte for some i.MX SoCs.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/nvmem/imx-ocotp.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 08f41328cc71..0b5a092ebcd2 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
+#include <dt-bindings/nvmem/nvmem.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
@@ -97,6 +98,7 @@ struct ocotp_params {
 	unsigned int bank_address_words;
 	void (*set_timing)(struct ocotp_priv *priv);
 	struct ocotp_ctrl_reg ctrl;
+	bool reverse_mac_address;
 };
 
 static int imx_ocotp_wait_for_busy(struct ocotp_priv *priv, u32 flags)
@@ -221,6 +223,29 @@ static int imx_ocotp_read(void *context, unsigned int offset,
 	return ret;
 }
 
+static int imx_ocotp_cell_pp(void *context, int type, unsigned int offset,
+			     void *data, size_t bytes)
+{
+	struct ocotp_priv *priv = context;
+
+	/* Deal with some post processing of nvmem cell data */
+	switch (type) {
+	case NVMEM_CELL_TYPE_MAC_ADDRESS:
+		if (priv->params->reverse_mac_address) {
+			u8 *buf = data;
+			int i;
+
+			for (i = 0; i < bytes/2; i++)
+				swap(buf[i], buf[bytes - i - 1]);
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv)
 {
 	unsigned long clk_rate;
@@ -468,6 +493,7 @@ static struct nvmem_config imx_ocotp_nvmem_config = {
 	.stride = 1,
 	.reg_read = imx_ocotp_read,
 	.reg_write = imx_ocotp_write,
+	.cell_post_process = imx_ocotp_cell_pp,
 };
 
 static const struct ocotp_params imx6q_params = {
@@ -530,6 +556,7 @@ static const struct ocotp_params imx8mq_params = {
 	.bank_address_words = 0,
 	.set_timing = imx_ocotp_set_imx6_timing,
 	.ctrl = IMX_OCOTP_BM_CTRL_DEFAULT,
+	.reverse_mac_address = true,
 };
 
 static const struct ocotp_params imx8mm_params = {
@@ -537,6 +564,7 @@ static const struct ocotp_params imx8mm_params = {
 	.bank_address_words = 0,
 	.set_timing = imx_ocotp_set_imx6_timing,
 	.ctrl = IMX_OCOTP_BM_CTRL_DEFAULT,
+	.reverse_mac_address = true,
 };
 
 static const struct ocotp_params imx8mn_params = {
@@ -544,6 +572,7 @@ static const struct ocotp_params imx8mn_params = {
 	.bank_address_words = 0,
 	.set_timing = imx_ocotp_set_imx6_timing,
 	.ctrl = IMX_OCOTP_BM_CTRL_DEFAULT,
+	.reverse_mac_address = true,
 };
 
 static const struct ocotp_params imx8mp_params = {
@@ -551,6 +580,7 @@ static const struct ocotp_params imx8mp_params = {
 	.bank_address_words = 0,
 	.set_timing = imx_ocotp_set_imx6_timing,
 	.ctrl = IMX_OCOTP_BM_CTRL_8MP,
+	.reverse_mac_address = true,
 };
 
 static const struct of_device_id imx_ocotp_dt_ids[] = {
-- 
2.17.1

[PATCH 5/6] arm64: dts: imx8mm: add "cell-type" property for mac-address

[Joakim Zhang]

Add "cell-type" property for mac-address nvmem cell to supporting mac
address reverse byte.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 ++
 arch/arm64/boot/dts/freescale/imx8mn.dtsi | 2 ++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 9 +++++++++
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 ++
 4 files changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index e7648c3b8390..fb14be932386 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/nvmem/nvmem.h>
 #include <dt-bindings/thermal/thermal.h>
 
 #include "imx8mm-pinfunc.h"
@@ -539,6 +540,7 @@
 
 				fec_mac_address: mac-address@90 {
 					reg = <0x90 6>;
+					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
 				};
 			};
 
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index d4231e061403..0a994e6edc0b 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/nvmem/nvmem.h>
 #include <dt-bindings/thermal/thermal.h>
 
 #include "imx8mn-pinfunc.h"
@@ -544,6 +545,7 @@
 
 				fec_mac_address: mac-address@90 {
 					reg = <0x90 6>;
+					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
 				};
 			};
 
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9f7c7f587d38..37188ff07f21 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/nvmem/nvmem.h>
 #include <dt-bindings/thermal/thermal.h>
 
 #include "imx8mp-pinfunc.h"
@@ -358,6 +359,12 @@
 
 				eth_mac1: mac-address@90 {
 					reg = <0x90 6>;
+					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
+				};
+
+				eth_mac2: mac-address@96 {
+					reg = <0x96 6>;
+					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
 				};
 			};
 
@@ -836,6 +843,8 @@
 							 <&clk IMX8MP_SYS_PLL2_100M>,
 							 <&clk IMX8MP_SYS_PLL2_125M>;
 				assigned-clock-rates = <0>, <100000000>, <125000000>;
+				nvmem-cells = <&eth_mac2>;
+				nvmem-cell-names = "mac-address";
 				intf_mode = <&gpr 0x4>;
 				status = "disabled";
 			};
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 91df9c5350ae..1cb211e470ae 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include "dt-bindings/input/input.h"
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/nvmem/nvmem.h>
 #include <dt-bindings/thermal/thermal.h>
 #include <dt-bindings/interconnect/imx8mq.h>
 #include "imx8mq-pinfunc.h"
@@ -570,6 +571,7 @@
 
 				fec_mac_address: mac-address@90 {
 					reg = <0x90 6>;
+					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
 				};
 			};
 
-- 
2.17.1

[PATCH 6/6] arm64: dts: imx8m: remove unused "nvmem_macaddr_swap" property for FEC

[Joakim Zhang]

Remove unused "nvmem_macaddr_swap" property for FEC, there is no info in both
dt-binding and driver, so it's safe to remove it.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 -
 arch/arm64/boot/dts/freescale/imx8mn.dtsi | 1 -
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 -
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 -
 4 files changed, 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index fb14be932386..2210cfda4e60 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -948,7 +948,6 @@
 				fsl,num-rx-queues = <3>;
 				nvmem-cells = <&fec_mac_address>;
 				nvmem-cell-names = "mac-address";
-				nvmem_macaddr_swap;
 				fsl,stop-mode = <&gpr 0x10 3>;
 				status = "disabled";
 			};
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index 0a994e6edc0b..408024426315 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -951,7 +951,6 @@
 				fsl,num-rx-queues = <3>;
 				nvmem-cells = <&fec_mac_address>;
 				nvmem-cell-names = "mac-address";
-				nvmem_macaddr_swap;
 				fsl,stop-mode = <&gpr 0x10 3>;
 				status = "disabled";
 			};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 37188ff07f21..cb7867791d05 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -821,7 +821,6 @@
 				nvmem-cells = <&eth_mac1>;
 				nvmem-cell-names = "mac-address";
 				fsl,stop-mode = <&gpr 0x10 3>;
-				nvmem_macaddr_swap;
 				status = "disabled";
 			};
 
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 1cb211e470ae..dc4e39ef9d39 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1191,7 +1191,6 @@
 				fsl,num-rx-queues = <3>;
 				nvmem-cells = <&fec_mac_address>;
 				nvmem-cell-names = "mac-address";
-				nvmem_macaddr_swap;
 				fsl,stop-mode = <&iomuxc_gpr 0x10 3>;
 				status = "disabled";
 			};
-- 
2.17.1

RE: [PATCH 0/6] nvmem: add "cell-type" property to support mac-address

[Joakim Zhang]

Gentle Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2021年9月8日 18:03
> To: srinivas.kandagatla@linaro.org; robh+dt@kernel.org;
> shawnguo@kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 0/6] nvmem: add "cell-type" property to support mac-address
> 
> This patch set adds "cell-type" property to parse mac address, take i.MX as an
> example, which need reverse byte for mac address.
> 
> Joakim Zhang (2):
>   arm64: dts: imx8mm: add "cell-type" property for mac-address
>   arm64: dts: imx8m: remove unused "nvmem_macaddr_swap" property for
> FEC
> 
> Srinivas Kandagatla (4):
>   dt-bindings: nvmem: add cell-type to nvmem cells
>   nvmem: core: parse nvmem cell-type from device tree
>   nvmem: core: add nvmem cell post processing callback
>   nvmem: imx-ocotp: add support for post porcessing.
> 
>  .../devicetree/bindings/nvmem/nvmem.yaml      | 11 +++++++
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  3 +-
>  arch/arm64/boot/dts/freescale/imx8mn.dtsi     |  3 +-
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi     | 10 ++++++-
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  3 +-
>  drivers/nvmem/core.c                          | 12 ++++++++
>  drivers/nvmem/imx-ocotp.c                     | 30
> +++++++++++++++++++
>  include/dt-bindings/nvmem/nvmem.h             |  8 +++++
>  include/linux/nvmem-provider.h                |  5 ++++
>  9 files changed, 81 insertions(+), 4 deletions(-)  create mode 100644
> include/dt-bindings/nvmem/nvmem.h
> 
> --
> 2.17.1

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Ahmad Fatoum]

Hi,

On 08.09.21 12:02, Joakim Zhang wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> Some of the nvmem providers encode data for certain type of nvmem cell,
> example mac-address is stored in ascii or with delimiter or in reverse order.
> 
> This is much specific to vendor, so having a cell-type would allow nvmem
> provider drivers to post-process this before using it.

I don't agree with this assessment. Users of the OCOTP so far
used this specific encoding. Bootloaders decode the OCOTP this way, but this
encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
with a different OTP IP will likely use the same format. Users may even
use the same format on an EEPROM to populate a second off-SoC interface, .. etc.

I'd thus prefer to not make this specific to the OCOTP as all:

  * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX	/* ... */

  * cell-type = <NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX>;

  * and then the decoder is placed into some generic location, e.g.
   drivers/nvmem/encodings.c for Linux

That way, we can reuse this and future encodings across nvmem providers.
It's also more extendable: e.g. big endian fields on EEPROMs. Just stick
the cell-type in, document it in the binding and drivers supporting it
will interpret bytes appropriately.

It's still a good idea to record the type as well as the encoding,
e.g. split the 32 bit encoding constant into two 16-bit values.
One is an enum of possible types (unknown, mac_address, IP address ... etc.)
and one is an enum of the available encodings.

What do you think?

> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
>  include/dt-bindings/nvmem/nvmem.h                  |  8 ++++++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 include/dt-bindings/nvmem/nvmem.h
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> index b8dc3d2b6e92..8cf6c7e72b0a 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> @@ -60,6 +60,11 @@ patternProperties:
>              - minimum: 1
>                description:
>                  Size in bit within the address range specified by reg.
> +      cell-type:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        maxItems: 1
> +        description:
> +          Type of nvmem, Use defines in dt-bindings/nvmem/nvmem.h.
>  
>      required:
>        - reg
> @@ -69,6 +74,7 @@ additionalProperties: true
>  examples:
>    - |
>        #include <dt-bindings/gpio/gpio.h>
> +      #include <dt-bindings/nvmem/nvmem.h>
>  
>        qfprom: eeprom@700000 {
>            #address-cells = <1>;
> @@ -98,6 +104,11 @@ examples:
>                reg = <0xc 0x1>;
>                bits = <2 3>;
>            };
> +
> +          mac_addr: mac-addr@90{
> +              reg = <0x90 0x6>;
> +              cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> +          };
>        };
>  
>  ...
> diff --git a/include/dt-bindings/nvmem/nvmem.h b/include/dt-bindings/nvmem/nvmem.h
> new file mode 100644
> index 000000000000..eed0478f6bfd
> --- /dev/null
> +++ b/include/dt-bindings/nvmem/nvmem.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __DT_NVMMEM_H
> +#define __DT_NVMMEM_H
> +
> +#define NVMEM_CELL_TYPE_UNKNOWN		0
> +#define NVMEM_CELL_TYPE_MAC_ADDRESS	1
> +
> +#endif /* __DT_NVMMEM_H */
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/6] nvmem: core: parse nvmem cell-type from device tree

[Ahmad Fatoum]

On 08.09.21 12:02, Joakim Zhang wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> get nvmem cell-type from device tree
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/nvmem/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 3d87fadaa160..23c08dbaf45e 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -52,6 +52,7 @@ struct nvmem_cell {
>  	int			bytes;
>  	int			bit_offset;
>  	int			nbits;
> +	u32			type;
>  	struct device_node	*np;
>  	struct nvmem_device	*nvmem;
>  	struct list_head	node;
> @@ -726,6 +727,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>  			return -EINVAL;
>  		}
>  
> +		of_property_read_u32(child, "cell-type", &cell->type);
> +
>  		cell->np = of_node_get(child);
>  		nvmem_cell_add(cell);
>  	}
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/6] nvmem: core: add nvmem cell post processing callback

[Ahmad Fatoum]

On 08.09.21 12:02, Joakim Zhang wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> Some NVMEM providers have certain nvmem cells encoded, which requires
> post processing before actually using it.
> 
> For example mac-address is stored in either in ascii or delimited or reverse-order.
> 
> Having a post-process callback hook to provider drivers would enable them to
> do this vendor specific post processing before nvmem consumers see it.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/nvmem/core.c           | 9 +++++++++
>  include/linux/nvmem-provider.h | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 23c08dbaf45e..4f81a3adf081 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -38,6 +38,7 @@ struct nvmem_device {
>  	unsigned int		nkeepout;
>  	nvmem_reg_read_t	reg_read;
>  	nvmem_reg_write_t	reg_write;
> +	nvmem_cell_post_process_t cell_post_process;
>  	struct gpio_desc	*wp_gpio;
>  	void *priv;
>  };
> @@ -797,6 +798,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	nvmem->type = config->type;
>  	nvmem->reg_read = config->reg_read;
>  	nvmem->reg_write = config->reg_write;
> +	nvmem->cell_post_process = config->cell_post_process;
>  	nvmem->keepout = config->keepout;
>  	nvmem->nkeepout = config->nkeepout;
>  	if (config->of_node)
> @@ -1404,6 +1406,13 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
>  	if (cell->bit_offset || cell->nbits)
>  		nvmem_shift_read_buffer_in_place(cell, buf);
>  
> +	if (nvmem->cell_post_process) {
> +		rc = nvmem->cell_post_process(nvmem->priv, cell->type,
> +					      cell->offset, buf, cell->bytes);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (len)
>  		*len = cell->bytes;
>  
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 104505e9028f..d980c79f9605 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -19,6 +19,9 @@ 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);
> +/* used for vendor specific post processing of cell data */
> +typedef int (*nvmem_cell_post_process_t)(void *priv, int type, unsigned int offset,
> +					  void *buf, size_t bytes);
>  
>  enum nvmem_type {
>  	NVMEM_TYPE_UNKNOWN = 0,
> @@ -62,6 +65,7 @@ struct nvmem_keepout {
>   * @no_of_node:	Device should not use the parent's of_node even if it's !NULL.
>   * @reg_read:	Callback to read data.
>   * @reg_write:	Callback to write data.
> + * @cell_read_callback: Callback for vendor specific post processing of cell data

The member below is called cell_post_process

>   * @size:	Device size.
>   * @word_size:	Minimum read/write access granularity.
>   * @stride:	Minimum read/write access stride.
> @@ -92,6 +96,7 @@ struct nvmem_config {
>  	bool			no_of_node;
>  	nvmem_reg_read_t	reg_read;
>  	nvmem_reg_write_t	reg_write;
> +	nvmem_cell_post_process_t cell_post_process;
>  	int	size;
>  	int	word_size;
>  	int	stride;
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 5/6] arm64: dts: imx8mm: add "cell-type" property for mac-address

[Ahmad Fatoum]

On 08.09.21 12:02, Joakim Zhang wrote:
> Add "cell-type" property for mac-address nvmem cell to supporting mac
> address reverse byte.

s/imx8mm/imx8m/ in commit message title.
Do you intend to do this for older i.MX as well?

> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 ++
>  arch/arm64/boot/dts/freescale/imx8mn.dtsi | 2 ++
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 9 +++++++++
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 ++
>  4 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index e7648c3b8390..fb14be932386 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -7,6 +7,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/nvmem/nvmem.h>
>  #include <dt-bindings/thermal/thermal.h>
>  
>  #include "imx8mm-pinfunc.h"
> @@ -539,6 +540,7 @@
>  
>  				fec_mac_address: mac-address@90 {
>  					reg = <0x90 6>;
> +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>  				};
>  			};
>  
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> index d4231e061403..0a994e6edc0b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> @@ -7,6 +7,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/nvmem/nvmem.h>
>  #include <dt-bindings/thermal/thermal.h>
>  
>  #include "imx8mn-pinfunc.h"
> @@ -544,6 +545,7 @@
>  
>  				fec_mac_address: mac-address@90 {
>  					reg = <0x90 6>;
> +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>  				};
>  			};
>  
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 9f7c7f587d38..37188ff07f21 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -7,6 +7,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/nvmem/nvmem.h>
>  #include <dt-bindings/thermal/thermal.h>
>  
>  #include "imx8mp-pinfunc.h"
> @@ -358,6 +359,12 @@
>  
>  				eth_mac1: mac-address@90 {
>  					reg = <0x90 6>;
> +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> +				};
> +
> +				eth_mac2: mac-address@96 {
> +					reg = <0x96 6>;
> +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>  				};
>  			};
>  
> @@ -836,6 +843,8 @@
>  							 <&clk IMX8MP_SYS_PLL2_100M>,
>  							 <&clk IMX8MP_SYS_PLL2_125M>;
>  				assigned-clock-rates = <0>, <100000000>, <125000000>;
> +				nvmem-cells = <&eth_mac2>;
> +				nvmem-cell-names = "mac-address";
>  				intf_mode = <&gpr 0x4>;
>  				status = "disabled";
>  			};
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 91df9c5350ae..1cb211e470ae 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include "dt-bindings/input/input.h"
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/nvmem/nvmem.h>
>  #include <dt-bindings/thermal/thermal.h>
>  #include <dt-bindings/interconnect/imx8mq.h>
>  #include "imx8mq-pinfunc.h"
> @@ -570,6 +571,7 @@
>  
>  				fec_mac_address: mac-address@90 {
>  					reg = <0x90 6>;
> +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>  				};
>  			};
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 6/6] arm64: dts: imx8m: remove unused "nvmem_macaddr_swap" property for FEC

[Ahmad Fatoum]

On 08.09.21 12:02, Joakim Zhang wrote:
> Remove unused "nvmem_macaddr_swap" property for FEC, there is no info in both
> dt-binding and driver, so it's safe to remove it.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 -
>  arch/arm64/boot/dts/freescale/imx8mn.dtsi | 1 -
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 -
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 -
>  4 files changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index fb14be932386..2210cfda4e60 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -948,7 +948,6 @@
>  				fsl,num-rx-queues = <3>;
>  				nvmem-cells = <&fec_mac_address>;
>  				nvmem-cell-names = "mac-address";
> -				nvmem_macaddr_swap;
>  				fsl,stop-mode = <&gpr 0x10 3>;
>  				status = "disabled";
>  			};
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> index 0a994e6edc0b..408024426315 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> @@ -951,7 +951,6 @@
>  				fsl,num-rx-queues = <3>;
>  				nvmem-cells = <&fec_mac_address>;
>  				nvmem-cell-names = "mac-address";
> -				nvmem_macaddr_swap;
>  				fsl,stop-mode = <&gpr 0x10 3>;
>  				status = "disabled";
>  			};
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 37188ff07f21..cb7867791d05 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -821,7 +821,6 @@
>  				nvmem-cells = <&eth_mac1>;
>  				nvmem-cell-names = "mac-address";
>  				fsl,stop-mode = <&gpr 0x10 3>;
> -				nvmem_macaddr_swap;
>  				status = "disabled";
>  			};
>  
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 1cb211e470ae..dc4e39ef9d39 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1191,7 +1191,6 @@
>  				fsl,num-rx-queues = <3>;
>  				nvmem-cells = <&fec_mac_address>;
>  				nvmem-cell-names = "mac-address";
> -				nvmem_macaddr_swap;
>  				fsl,stop-mode = <&iomuxc_gpr 0x10 3>;
>  				status = "disabled";
>  			};
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Srinivas Kandagatla]

On 22/09/2021 12:34, Ahmad Fatoum wrote:
> Hi,
> 
> On 08.09.21 12:02, Joakim Zhang wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> Some of the nvmem providers encode data for certain type of nvmem cell,
>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>
>> This is much specific to vendor, so having a cell-type would allow nvmem
>> provider drivers to post-process this before using it.
> 
> I don't agree with this assessment. Users of the OCOTP so far
> used this specific encoding. Bootloaders decode the OCOTP this way, but this
> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
> with a different OTP IP will likely use the same format. Users may even
> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
> 

That is okay.

> I'd thus prefer to not make this specific to the OCOTP as all:
> 
>    * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX	/* ... */
> 
>    * cell-type = <NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX>;
> 

No, this is not okay, cell-type is just representing what is the cell 
type in a generic way, and its not intended to have any encoding 
information.

Encoding information should be derived from the provider specific 
bindings. If we start adding this info in generic binding we will endup 
with a mess.
And this has been discussed in various instances.

>    * and then the decoder is placed into some generic location, e.g.
>     drivers/nvmem/encodings.c for Linux

This is fine, we could have a library to do these post-processing, but 
only if we have enough users. For now its better to keep it within 
provider drivers till we have more consumers of these functions.


--srini
> 
> That way, we can reuse this and future encodings across nvmem providers.
> It's also more extendable: e.g. big endian fields on EEPROMs. Just stick
> the cell-type in, document it in the binding and drivers supporting it
> will interpret bytes appropriately.
> 
> It's still a good idea to record the type as well as the encoding,
> e.g. split the 32 bit encoding constant into two 16-bit values.
> One is an enum of possible types (unknown, mac_address, IP address ... etc.)
> and one is an enum of the available encodings.
> 
> What do you think?
> 
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> ---
>>   Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
>>   include/dt-bindings/nvmem/nvmem.h                  |  8 ++++++++
>>   2 files changed, 19 insertions(+)
>>   create mode 100644 include/dt-bindings/nvmem/nvmem.h
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> index b8dc3d2b6e92..8cf6c7e72b0a 100644
>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> @@ -60,6 +60,11 @@ patternProperties:
>>               - minimum: 1
>>                 description:
>>                   Size in bit within the address range specified by reg.
>> +      cell-type:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        maxItems: 1
>> +        description:
>> +          Type of nvmem, Use defines in dt-bindings/nvmem/nvmem.h.
>>   
>>       required:
>>         - reg
>> @@ -69,6 +74,7 @@ additionalProperties: true
>>   examples:
>>     - |
>>         #include <dt-bindings/gpio/gpio.h>
>> +      #include <dt-bindings/nvmem/nvmem.h>
>>   
>>         qfprom: eeprom@700000 {
>>             #address-cells = <1>;
>> @@ -98,6 +104,11 @@ examples:
>>                 reg = <0xc 0x1>;
>>                 bits = <2 3>;
>>             };
>> +
>> +          mac_addr: mac-addr@90{
>> +              reg = <0x90 0x6>;
>> +              cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>> +          };
>>         };
>>   
>>   ...
>> diff --git a/include/dt-bindings/nvmem/nvmem.h b/include/dt-bindings/nvmem/nvmem.h
>> new file mode 100644
>> index 000000000000..eed0478f6bfd
>> --- /dev/null
>> +++ b/include/dt-bindings/nvmem/nvmem.h
>> @@ -0,0 +1,8 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __DT_NVMMEM_H
>> +#define __DT_NVMMEM_H
>> +
>> +#define NVMEM_CELL_TYPE_UNKNOWN		0
>> +#define NVMEM_CELL_TYPE_MAC_ADDRESS	1
>> +
>> +#endif /* __DT_NVMMEM_H */
>>
> 
> 

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Ahmad Fatoum]

Hello Srini,

On 22.09.21 14:23, Srinivas Kandagatla wrote:
> 
> 
> On 22/09/2021 12:34, Ahmad Fatoum wrote:
>> Hi,
>>
>> On 08.09.21 12:02, Joakim Zhang wrote:
>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>
>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>
>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>> provider drivers to post-process this before using it.
>>
>> I don't agree with this assessment. Users of the OCOTP so far
>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>> with a different OTP IP will likely use the same format. Users may even
>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>
> 
> That is okay.

How would you go about using this same format on an EEPROM?

>> I'd thus prefer to not make this specific to the OCOTP as all:
>>
>>    * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
>>
>>    * cell-type = <NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX>;
>>
> 
> No, this is not okay, cell-type is just representing what is the cell type in a generic way, and its not intended to have any encoding information.
> 
> Encoding information should be derived from the provider specific bindings. If we start adding this info in generic binding we will endup with a mess.
> And this has been discussed in various instances.

A linux-nvmem list would be nice to collect such discussions.

>>    * and then the decoder is placed into some generic location, e.g.
>>     drivers/nvmem/encodings.c for Linux
> 
> This is fine, we could have a library to do these post-processing, but only if we have enough users. For now its better to keep it within provider drivers till we have more consumers of these functions.
> 
> 
> --srini
>>
>> That way, we can reuse this and future encodings across nvmem providers.
>> It's also more extendable: e.g. big endian fields on EEPROMs. Just stick
>> the cell-type in, document it in the binding and drivers supporting it
>> will interpret bytes appropriately.
>>
>> It's still a good idea to record the type as well as the encoding,
>> e.g. split the 32 bit encoding constant into two 16-bit values.
>> One is an enum of possible types (unknown, mac_address, IP address ... etc.)
>> and one is an enum of the available encodings.
>>
>> What do you think?
>>
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>> ---
>>>   Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
>>>   include/dt-bindings/nvmem/nvmem.h                  |  8 ++++++++
>>>   2 files changed, 19 insertions(+)
>>>   create mode 100644 include/dt-bindings/nvmem/nvmem.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> index b8dc3d2b6e92..8cf6c7e72b0a 100644
>>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> @@ -60,6 +60,11 @@ patternProperties:
>>>               - minimum: 1
>>>                 description:
>>>                   Size in bit within the address range specified by reg.
>>> +      cell-type:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        maxItems: 1
>>> +        description:
>>> +          Type of nvmem, Use defines in dt-bindings/nvmem/nvmem.h.
>>>         required:
>>>         - reg
>>> @@ -69,6 +74,7 @@ additionalProperties: true
>>>   examples:
>>>     - |
>>>         #include <dt-bindings/gpio/gpio.h>
>>> +      #include <dt-bindings/nvmem/nvmem.h>
>>>           qfprom: eeprom@700000 {
>>>             #address-cells = <1>;
>>> @@ -98,6 +104,11 @@ examples:
>>>                 reg = <0xc 0x1>;
>>>                 bits = <2 3>;
>>>             };
>>> +
>>> +          mac_addr: mac-addr@90{
>>> +              reg = <0x90 0x6>;
>>> +              cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>>> +          };
>>>         };
>>>     ...
>>> diff --git a/include/dt-bindings/nvmem/nvmem.h b/include/dt-bindings/nvmem/nvmem.h
>>> new file mode 100644
>>> index 000000000000..eed0478f6bfd
>>> --- /dev/null
>>> +++ b/include/dt-bindings/nvmem/nvmem.h
>>> @@ -0,0 +1,8 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef __DT_NVMMEM_H
>>> +#define __DT_NVMMEM_H
>>> +
>>> +#define NVMEM_CELL_TYPE_UNKNOWN        0
>>> +#define NVMEM_CELL_TYPE_MAC_ADDRESS    1
>>> +
>>> +#endif /* __DT_NVMMEM_H */
>>>
>>
>>
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Srinivas Kandagatla]

On 22/09/2021 13:31, Ahmad Fatoum wrote:
>>>
>>> On 08.09.21 12:02, Joakim Zhang wrote:
>>>> From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>>
>>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>>
>>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>>> provider drivers to post-process this before using it.
>>> I don't agree with this assessment. Users of the OCOTP so far
>>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>>> with a different OTP IP will likely use the same format. Users may even
>>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>>
>> That is okay.
> How would you go about using this same format on an EEPROM?

Am guessing that by the time there are more users for such formats, 
those post-processing functions should be converted into some library 
functions.

--srini

> 
>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>
>>>     * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Ahmad Fatoum]

Hi Srini,

On 22.09.21 14:49, Srinivas Kandagatla wrote:
> 
> 
> On 22/09/2021 13:31, Ahmad Fatoum wrote:
>>>>
>>>> On 08.09.21 12:02, Joakim Zhang wrote:
>>>>> From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>>>
>>>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>>>
>>>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>>>> provider drivers to post-process this before using it.
>>>> I don't agree with this assessment. Users of the OCOTP so far
>>>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>>>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>>>> with a different OTP IP will likely use the same format. Users may even
>>>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>>>
>>> That is okay.
>> How would you go about using this same format on an EEPROM?
> 
> Am guessing that by the time there are more users for such formats, those post-processing functions should be converted into some library functions.

User A wants to reverse bytes in MAC address. User B stores it in ASCII.
Both use the exact same EEPROM. How could this ever work when the
encoding decision is left to the EEPROM driver?

> 
> --srini
> 
>>
>>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>>
>>>>     * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Srinivas Kandagatla]

On 22/09/2021 13:58, Ahmad Fatoum wrote:
> Hi Srini,
> 
> On 22.09.21 14:49, Srinivas Kandagatla wrote:
>>
>>
>> On 22/09/2021 13:31, Ahmad Fatoum wrote:
>>>>>
>>>>> On 08.09.21 12:02, Joakim Zhang wrote:
>>>>>> From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>>>>
>>>>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>>>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>>>>
>>>>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>>>>> provider drivers to post-process this before using it.
>>>>> I don't agree with this assessment. Users of the OCOTP so far
>>>>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>>>>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>>>>> with a different OTP IP will likely use the same format. Users may even
>>>>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>>>>
>>>> That is okay.
>>> How would you go about using this same format on an EEPROM?
>>
>> Am guessing that by the time there are more users for such formats, those post-processing functions should be converted into some library functions.
> 
> User A wants to reverse bytes in MAC address. User B stores it in ASCII.
> Both use the exact same EEPROM. How could this ever work when the
> encoding decision is left to the EEPROM driver?

User A and B should mention about this encoding information in there 
NVMEM provider bindings.

Based on that specific post-processing should be selected.

--srini
> 

>>
>> --srini
>>
>>>
>>>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>>>
>>>>>      * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
>>
> 
> 

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Ahmad Fatoum]

Hi,

On 22.09.21 15:03, Srinivas Kandagatla wrote:
> 
> 
> On 22/09/2021 13:58, Ahmad Fatoum wrote:
>> Hi Srini,
>>
>> On 22.09.21 14:49, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 22/09/2021 13:31, Ahmad Fatoum wrote:
>>>>>>
>>>>>> On 08.09.21 12:02, Joakim Zhang wrote:
>>>>>>> From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>>>>>
>>>>>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>>>>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>>>>>
>>>>>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>>>>>> provider drivers to post-process this before using it.
>>>>>> I don't agree with this assessment. Users of the OCOTP so far
>>>>>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>>>>>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>>>>>> with a different OTP IP will likely use the same format. Users may even
>>>>>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>>>>>
>>>>> That is okay.
>>>> How would you go about using this same format on an EEPROM?
>>>
>>> Am guessing that by the time there are more users for such formats, those post-processing functions should be converted into some library functions.
>>
>> User A wants to reverse bytes in MAC address. User B stores it in ASCII.
>> Both use the exact same EEPROM. How could this ever work when the
>> encoding decision is left to the EEPROM driver?
> 
> User A and B should mention about this encoding information in there NVMEM provider bindings.
> 
> Based on that specific post-processing should be selected.

So instead of just compatible = "atmel,at24c16"; there will be

  compatible = "user-A,my-eeprom", "atmel,at24c16";

and 

  compatible = "user-B,my-eeprom", "atmel,at24c16";

and they each need to patch the at24 driver to call one of the
common library functions?

> 
> --srini
>>
> 
>>>
>>> --srini
>>>
>>>>
>>>>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>>>>
>>>>>>      * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
>>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Srinivas Kandagatla]

On 22/09/2021 14:08, Ahmad Fatoum wrote:
> Hi,
> 
> On 22.09.21 15:03, Srinivas Kandagatla wrote:
>>
>>
>> On 22/09/2021 13:58, Ahmad Fatoum wrote:
>>> Hi Srini,
>>>
>>> On 22.09.21 14:49, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 22/09/2021 13:31, Ahmad Fatoum wrote:
>>>>>>>
>>>>>>> On 08.09.21 12:02, Joakim Zhang wrote:
>>>>>>>> From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>>>>>>
>>>>>>>> Some of the nvmem providers encode data for certain type of nvmem cell,
>>>>>>>> example mac-address is stored in ascii or with delimiter or in reverse order.
>>>>>>>>
>>>>>>>> This is much specific to vendor, so having a cell-type would allow nvmem
>>>>>>>> provider drivers to post-process this before using it.
>>>>>>> I don't agree with this assessment. Users of the OCOTP so far
>>>>>>> used this specific encoding. Bootloaders decode the OCOTP this way, but this
>>>>>>> encoding isn't really an inherent attribute of the OCOTP. A new NXP SoC
>>>>>>> with a different OTP IP will likely use the same format. Users may even
>>>>>>> use the same format on an EEPROM to populate a second off-SoC interface, .. etc.
>>>>>>>
>>>>>> That is okay.
>>>>> How would you go about using this same format on an EEPROM?
>>>>
>>>> Am guessing that by the time there are more users for such formats, those post-processing functions should be converted into some library functions.
>>>
>>> User A wants to reverse bytes in MAC address. User B stores it in ASCII.
>>> Both use the exact same EEPROM. How could this ever work when the
>>> encoding decision is left to the EEPROM driver?
>>
>> User A and B should mention about this encoding information in there NVMEM provider bindings.
>>
>> Based on that specific post-processing should be selected.
> 
> So instead of just compatible = "atmel,at24c16"; there will be
> 
>    compatible = "user-A,my-eeprom", "atmel,at24c16";
> 
> and
> 
>    compatible = "user-B,my-eeprom", "atmel,at24c16";

It will depend how you specify encoding information.

The issue with making this encoding information generic is that the 
combinations are endless and nvmem core bindings is definitely not the 
right place for this.

ex:
If I remember correctly we have mac-address stored in various formats:
from old thread I can see

Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)

Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so 
on) (Swapped/non-Swapped)

Type 3: Is the one which stores mac address in Type1/2 but this has to
be incremented to be used on other instances of eth.

Type 4: Octets as bytes/u8, swapped/non-swapped

This list can be endless and its not just the cell-type property you 
have to deal with, new properties keep popping up.


--srini



> 
> and they each need to patch the at24 driver to call one of the
> common library functions?
> 
>>
>> --srini
>>>
>>
>>>>
>>>> --srini
>>>>
>>>>>
>>>>>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>>>>>
>>>>>>>       * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
>>>>
>>>
>>>
>>
> 
> 

RE: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Joakim Zhang]

Hi Ahmad,

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: 2021年9月22日 19:34
> To: Joakim Zhang <qiangqing.zhang@nxp.com>;
> srinivas.kandagatla@linaro.org; robh+dt@kernel.org; shawnguo@kernel.org;
> Jan Lübbe <jlu@pengutronix.de>
> Cc: devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells
> 
> Hi,
> 
> On 08.09.21 12:02, Joakim Zhang wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >
> > Some of the nvmem providers encode data for certain type of nvmem
> > cell, example mac-address is stored in ascii or with delimiter or in reverse
> order.
> >
> > This is much specific to vendor, so having a cell-type would allow
> > nvmem provider drivers to post-process this before using it.
> 
> I don't agree with this assessment. Users of the OCOTP so far used this specific
> encoding. Bootloaders decode the OCOTP this way, but this encoding isn't
> really an inherent attribute of the OCOTP. A new NXP SoC with a different OTP
> IP will likely use the same format. Users may even use the same format on an
> EEPROM to populate a second off-SoC interface, .. etc.
> 
> I'd thus prefer to not make this specific to the OCOTP as all:
> 
>   * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX	/* ... */
> 
>   * cell-type = <NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX>;
> 
>   * and then the decoder is placed into some generic location, e.g.
>    drivers/nvmem/encodings.c for Linux
> 
> That way, we can reuse this and future encodings across nvmem providers.
> It's also more extendable: e.g. big endian fields on EEPROMs. Just stick the
> cell-type in, document it in the binding and drivers supporting it will interpret
> bytes appropriately.
> 
> It's still a good idea to record the type as well as the encoding, e.g. split the 32
> bit encoding constant into two 16-bit values.
> One is an enum of possible types (unknown, mac_address, IP address ... etc.)
> and one is an enum of the available encodings.
> 
> What do you think?

Go through the thread you discussed with Srinivas, as we discussed before, we prefer to offload this decoding to
specific nvmem provider driver, instead of nvmem core. 

Best Regards,
Joakim Zhang

RE: [PATCH 3/6] nvmem: core: add nvmem cell post processing callback

[Joakim Zhang]

Hi Ahmad,

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: 2021年9月22日 19:37
> To: Joakim Zhang <qiangqing.zhang@nxp.com>;
> srinivas.kandagatla@linaro.org; robh+dt@kernel.org; shawnguo@kernel.org
> Cc: devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/6] nvmem: core: add nvmem cell post processing
> callback
> 
> On 08.09.21 12:02, Joakim Zhang wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >
> > Some NVMEM providers have certain nvmem cells encoded, which requires
> > post processing before actually using it.
> >
> > For example mac-address is stored in either in ascii or delimited or
> reverse-order.
> >
> > Having a post-process callback hook to provider drivers would enable
> > them to do this vendor specific post processing before nvmem consumers see
> it.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/nvmem/core.c           | 9 +++++++++
> >  include/linux/nvmem-provider.h | 5 +++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index
> > 23c08dbaf45e..4f81a3adf081 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -38,6 +38,7 @@ struct nvmem_device {
> >  	unsigned int		nkeepout;
> >  	nvmem_reg_read_t	reg_read;
> >  	nvmem_reg_write_t	reg_write;
> > +	nvmem_cell_post_process_t cell_post_process;
> >  	struct gpio_desc	*wp_gpio;
> >  	void *priv;
> >  };
> > @@ -797,6 +798,7 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> >  	nvmem->type = config->type;
> >  	nvmem->reg_read = config->reg_read;
> >  	nvmem->reg_write = config->reg_write;
> > +	nvmem->cell_post_process = config->cell_post_process;
> >  	nvmem->keepout = config->keepout;
> >  	nvmem->nkeepout = config->nkeepout;
> >  	if (config->of_node)
> > @@ -1404,6 +1406,13 @@ static int __nvmem_cell_read(struct
> nvmem_device *nvmem,
> >  	if (cell->bit_offset || cell->nbits)
> >  		nvmem_shift_read_buffer_in_place(cell, buf);
> >
> > +	if (nvmem->cell_post_process) {
> > +		rc = nvmem->cell_post_process(nvmem->priv, cell->type,
> > +					      cell->offset, buf, cell->bytes);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> >  	if (len)
> >  		*len = cell->bytes;
> >
> > diff --git a/include/linux/nvmem-provider.h
> > b/include/linux/nvmem-provider.h index 104505e9028f..d980c79f9605
> > 100644
> > --- a/include/linux/nvmem-provider.h
> > +++ b/include/linux/nvmem-provider.h
> > @@ -19,6 +19,9 @@ 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);
> > +/* used for vendor specific post processing of cell data */ typedef
> > +int (*nvmem_cell_post_process_t)(void *priv, int type, unsigned int offset,
> > +					  void *buf, size_t bytes);
> >
> >  enum nvmem_type {
> >  	NVMEM_TYPE_UNKNOWN = 0,
> > @@ -62,6 +65,7 @@ struct nvmem_keepout {
> >   * @no_of_node:	Device should not use the parent's of_node even if
> it's !NULL.
> >   * @reg_read:	Callback to read data.
> >   * @reg_write:	Callback to write data.
> > + * @cell_read_callback: Callback for vendor specific post processing
> > + of cell data
> 
> The member below is called cell_post_process

Good caught! Will fix it in V2. Thanks.

Best Regards,
Joakim Zhang
> >   * @size:	Device size.
> >   * @word_size:	Minimum read/write access granularity.
> >   * @stride:	Minimum read/write access stride.
> > @@ -92,6 +96,7 @@ struct nvmem_config {
> >  	bool			no_of_node;
> >  	nvmem_reg_read_t	reg_read;
> >  	nvmem_reg_write_t	reg_write;
> > +	nvmem_cell_post_process_t cell_post_process;
> >  	int	size;
> >  	int	word_size;
> >  	int	stride;
> >
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7C85
> 4dcad461914aa4227e08d97dbd56f0%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637679074424671648%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&amp;sdata=nYxnuFbFB9PhGfEUPiwg7dJOR2hQHV5NbjfT1NyKWBM%3D&a
> mp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

RE: [PATCH 5/6] arm64: dts: imx8mm: add "cell-type" property for mac-address

[Joakim Zhang]

Hi Ahmad,

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: 2021年9月22日 19:40
> To: Joakim Zhang <qiangqing.zhang@nxp.com>;
> srinivas.kandagatla@linaro.org; robh+dt@kernel.org; shawnguo@kernel.org
> Cc: devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/6] arm64: dts: imx8mm: add "cell-type" property for
> mac-address
> 
> On 08.09.21 12:02, Joakim Zhang wrote:
> > Add "cell-type" property for mac-address nvmem cell to supporting mac
> > address reverse byte.
> 
> s/imx8mm/imx8m/ in commit message title.

Will fix it in V2.

> Do you intend to do this for older i.MX as well?

No plan now, I just prepare it for i.MX8M serials.

Best Regards,
Joakim Zhang
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 ++
> > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 2 ++
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 9 +++++++++
> > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 ++
> >  4 files changed, 15 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index e7648c3b8390..fb14be932386 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/nvmem/nvmem.h>
> >  #include <dt-bindings/thermal/thermal.h>
> >
> >  #include "imx8mm-pinfunc.h"
> > @@ -539,6 +540,7 @@
> >
> >  				fec_mac_address: mac-address@90 {
> >  					reg = <0x90 6>;
> > +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> >  				};
> >  			};
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > index d4231e061403..0a994e6edc0b 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/nvmem/nvmem.h>
> >  #include <dt-bindings/thermal/thermal.h>
> >
> >  #include "imx8mn-pinfunc.h"
> > @@ -544,6 +545,7 @@
> >
> >  				fec_mac_address: mac-address@90 {
> >  					reg = <0x90 6>;
> > +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> >  				};
> >  			};
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index 9f7c7f587d38..37188ff07f21 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/nvmem/nvmem.h>
> >  #include <dt-bindings/thermal/thermal.h>
> >
> >  #include "imx8mp-pinfunc.h"
> > @@ -358,6 +359,12 @@
> >
> >  				eth_mac1: mac-address@90 {
> >  					reg = <0x90 6>;
> > +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> > +				};
> > +
> > +				eth_mac2: mac-address@96 {
> > +					reg = <0x96 6>;
> > +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> >  				};
> >  			};
> >
> > @@ -836,6 +843,8 @@
> >  							 <&clk IMX8MP_SYS_PLL2_100M>,
> >  							 <&clk IMX8MP_SYS_PLL2_125M>;
> >  				assigned-clock-rates = <0>, <100000000>, <125000000>;
> > +				nvmem-cells = <&eth_mac2>;
> > +				nvmem-cell-names = "mac-address";
> >  				intf_mode = <&gpr 0x4>;
> >  				status = "disabled";
> >  			};
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > index 91df9c5350ae..1cb211e470ae 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > @@ -10,6 +10,7 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include "dt-bindings/input/input.h"
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/nvmem/nvmem.h>
> >  #include <dt-bindings/thermal/thermal.h>  #include
> > <dt-bindings/interconnect/imx8mq.h>
> >  #include "imx8mq-pinfunc.h"
> > @@ -570,6 +571,7 @@
> >
> >  				fec_mac_address: mac-address@90 {
> >  					reg = <0x90 6>;
> > +					cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> >  				};
> >  			};
> >
> >
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7C94
> d6744dc99545da696a08d97dbdb83f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637679076057590614%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&amp;sdata=rb9ZZXEqNxQaq9PORDP5ZVOgqZINFltBG1eMvM9VcBc%3D&a
> mp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

Re: [PATCH 1/6] dt-bindings: nvmem: add cell-type to nvmem cells

[Ahmad Fatoum]

Hello Srini,

On 22.09.21 15:23, Srinivas Kandagatla wrote:
> On 22/09/2021 14:08, Ahmad Fatoum wrote:
>> On 22.09.21 15:03, Srinivas Kandagatla wrote:
>>> User A and B should mention about this encoding information in there NVMEM provider bindings.
>>>
>>> Based on that specific post-processing should be selected.
>>
>> So instead of just compatible = "atmel,at24c16"; there will be
>>
>>    compatible = "user-A,my-eeprom", "atmel,at24c16";
>>
>> and
>>
>>    compatible = "user-B,my-eeprom", "atmel,at24c16";
> 
> It will depend how you specify encoding information.

I would specify them in cell-type, which you disagree with, thus
I am asking:

Is using a stock EEPROM with a non-canonical format for e.g. a MAC
address something that you think should be supported with NVMEM?

> The issue with making this encoding information generic is that the combinations are endless and nvmem core bindings is definitely not the right place for this.

Add a separate file and include it from the core file?

> ex:
> If I remember correctly we have mac-address stored in various formats:
> from old thread I can see
> 
> Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)
> 
> Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on) (Swapped/non-Swapped)
> 
> Type 3: Is the one which stores mac address in Type1/2 but this has to
> be incremented to be used on other instances of eth.
> 
> Type 4: Octets as bytes/u8, swapped/non-swapped
> 
> This list can be endless and its not just the cell-type property you have to deal with, new properties keep popping up.

Yes, an extendible interface will likely encourage people to extend it.

Cheers,
Ahmad

> --srini
> 
> 
> 
>>
>> and they each need to patch the at24 driver to call one of the
>> common library functions?
>>
>>>
>>> --srini
>>>>
>>>
>>>>>
>>>>> --srini
>>>>>
>>>>>>
>>>>>>>> I'd thus prefer to not make this specific to the OCOTP as all:
>>>>>>>>
>>>>>>>>       * #define NVMEM_CELL_ENCODING_MAC_ADDRESS_IMX    /* ... */
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |