[PATCH 0/5] Add ZynqMP efuse access support

[PATCH 0/5] Add ZynqMP efuse access support

[Praveen Teja Kundanala]

Add following support
 - ZynqMP efuse firmware API for efuse access
 - Convert txt to yaml file
 - Add nodes for ZynqMP efuses in yaml file
 - Add device tree(DT) nodes for nvmem access
 - Update driver to provide support to
    read/write ZynqMP efuse memory

Praveen Teja Kundanala (5):
  firmware: xilinx: Add ZynqMP efuse access API
  dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml
  dt-bindings: nvmem: Add nodes for ZynqMP efuses
  arm64: zynqmp: Add ZynqnMP nvmem nodes
  nvmem: zynqmp_nvmem: Add support to access efuse

 .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      |  46 ---
 .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 270 ++++++++++++++++++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        |  55 ++++
 drivers/firmware/xilinx/zynqmp.c              |  25 ++
 drivers/nvmem/zynqmp_nvmem.c                  | 216 ++++++++++++--
 include/linux/firmware/xlnx-zynqmp.h          |   8 +
 6 files changed, 543 insertions(+), 77 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
 create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml

-- 
2.36.1

[PATCH 1/5] firmware: xilinx: Add ZynqMP efuse access API

[Praveen Teja Kundanala]

Add zynqmp_pm_efuse_access API in the ZynqMP
firmware for read/write access of efuse memory.

Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 25 +++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h |  8 ++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index f8c4eb2b43f8..b0f6272e0844 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -3,6 +3,7 @@
  * Xilinx Zynq MPSoC Firmware layer
  *
  *  Copyright (C) 2014-2022 Xilinx, Inc.
+ *  Copyright (C), 2022 - 2023 Advanced Micro Devices, Inc.
  *
  *  Michal Simek <michal.simek@amd.com>
  *  Davorin Mista <davorin.mista@aggios.com>
@@ -1390,6 +1391,30 @@ int zynqmp_pm_aes_engine(const u64 address, u32 *out)
 }
 EXPORT_SYMBOL_GPL(zynqmp_pm_aes_engine);
 
+/**
+ * zynqmp_pm_efuse_access - Provides access to efuse memory.
+ * @address:	Address of the efuse params structure
+ * @out:		Returned output value
+ *
+ * Return:	Returns status, either success or error code.
+ */
+int zynqmp_pm_efuse_access(const u64 address, u32 *out)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	int ret;
+
+	if (!out)
+		return -EINVAL;
+
+	ret = zynqmp_pm_invoke_fn(PM_EFUSE_ACCESS, upper_32_bits(address),
+				  lower_32_bits(address), 0, 0,
+				  ret_payload);
+	*out = ret_payload[1];
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_efuse_access);
+
 /**
  * zynqmp_pm_sha_hash - Access the SHA engine to calculate the hash
  * @address:	Address of the data/ Address of output buffer where
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 9dda7d9898ff..721cebae3f14 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -3,6 +3,7 @@
  * Xilinx Zynq MPSoC Firmware layer
  *
  *  Copyright (C) 2014-2021 Xilinx
+ *  Copyright (C), 2022 - 2023 Advanced Micro Devices, Inc.
  *
  *  Michal Simek <michal.simek@amd.com>
  *  Davorin Mista <davorin.mista@aggios.com>
@@ -130,6 +131,7 @@ enum pm_api_id {
 	PM_CLOCK_GETPARENT = 44,
 	PM_FPGA_READ = 46,
 	PM_SECURE_AES = 47,
+	PM_EFUSE_ACCESS = 53,
 	PM_FEATURE_CHECK = 63,
 };
 
@@ -521,6 +523,7 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 			      const u32 qos,
 			      const enum zynqmp_pm_request_ack ack);
 int zynqmp_pm_aes_engine(const u64 address, u32 *out);
+int zynqmp_pm_efuse_access(const u64 address, u32 *out);
 int zynqmp_pm_sha_hash(const u64 address, const u32 size, const u32 flags);
 int zynqmp_pm_fpga_load(const u64 address, const u32 size, const u32 flags);
 int zynqmp_pm_fpga_get_status(u32 *value);
@@ -714,6 +717,11 @@ static inline int zynqmp_pm_aes_engine(const u64 address, u32 *out)
 	return -ENODEV;
 }
 
+static inline int zynqmp_pm_efuse_access(const u64 address, u32 *out)
+{
+	return -ENODEV;
+}
+
 static inline int zynqmp_pm_sha_hash(const u64 address, const u32 size,
 				     const u32 flags)
 {
-- 
2.36.1

[PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Praveen Teja Kundanala]

Convert the xlnx,zynqmp-nvmem.txt to yaml.

Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
 .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      | 46 ---------------
 .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 59 +++++++++++++++++++
 2 files changed, 59 insertions(+), 46 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
 create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
deleted file mode 100644
index 4881561b3a02..000000000000
--- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
+++ /dev/null
@@ -1,46 +0,0 @@
---------------------------------------------------------------------------
-=  Zynq UltraScale+ MPSoC nvmem firmware driver binding =
---------------------------------------------------------------------------
-The nvmem_firmware node provides access to the hardware related data
-like soc revision, IDCODE... etc, By using the firmware interface.
-
-Required properties:
-- compatible: should be "xlnx,zynqmp-nvmem-fw"
-
-= Data cells =
-Are child nodes of silicon id, bindings of which as described in
-bindings/nvmem/nvmem.txt
-
--------
- Example
--------
-firmware {
-	zynqmp_firmware: zynqmp-firmware {
-		compatible = "xlnx,zynqmp-firmware";
-		method = "smc";
-
-		nvmem_firmware {
-			compatible = "xlnx,zynqmp-nvmem-fw";
-			#address-cells = <1>;
-			#size-cells = <1>;
-
-			/* Data cells */
-			soc_revision: soc_revision {
-				reg = <0x0 0x4>;
-			};
-		};
-	};
-};
-
-= Data consumers =
-Are device nodes which consume nvmem data cells.
-
-For example:
-	pcap {
-		...
-
-		nvmem-cells = <&soc_revision>;
-		nvmem-cell-names = "soc_revision";
-
-		...
-	};
diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
new file mode 100644
index 000000000000..e03ed8c32537
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
+
+description: |
+    The ZynqMP MPSoC provides access to the hardware related data
+    like SOC revision, IDCODE.
+
+maintainers:
+  - Kalyani Akula <kalyani.akula@amd.com>
+  - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
+
+allOf:
+  - $ref: "nvmem.yaml#"
+
+properties:
+  compatible:
+    const: xlnx,zynqmp-nvmem-fw
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
+required:
+  - compatible
+
+patternProperties:
+  "^soc_revision@0$":
+    type: object
+    description:
+      This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    nvmem_firmware {
+        compatible = "xlnx,zynqmp-nvmem-fw";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        /* Data cells */
+        soc_revision: soc_revision@0 {
+            reg = <0x0 0x4>;
+        };
+    };
-- 
2.36.1

[PATCH 3/5] dt-bindings: nvmem: Add nodes for ZynqMP efuses

[Praveen Teja Kundanala]

Added nodes for ZynqMP specific purpose and PUF user efuses

Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
 .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 213 +++++++++++++++++-
 1 file changed, 212 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
index e03ed8c32537..d2a036a80cda 100644
--- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
@@ -8,7 +8,7 @@ title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
 
 description: |
     The ZynqMP MPSoC provides access to the hardware related data
-    like SOC revision, IDCODE.
+    like SOC revision, IDCODE and specific purpose efuses.
 
 maintainers:
   - Kalyani Akula <kalyani.akula@amd.com>
@@ -43,6 +43,140 @@ patternProperties:
     required:
       - reg
 
+  "^efuse_dna@c$":
+    type: object
+    description:
+      This node is used to read DNA of ZynqMP SOC. Read-only.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_usr(0@20|1@24|2@28|3@2c|4@30|5@34|6@38|7@3c)$":
+    type: object
+    description:
+      Eight 32-bit user efuses. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_miscusr@40$":
+    type: object
+    description:
+      32-bit MISC user efuse space. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_chash@50$":
+    type: object
+    description:
+      32-bit PUF chash space. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_pufmisc@54$":
+    type: object
+    description:
+      32-bit PUF MISC control space. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_sec@58$":
+    type: object
+    description:
+      32-bit secure control space. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_spkid@5c$":
+    type: object
+    description:
+      32-bit SPK ID. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_aeskey@60$":
+    type: object
+    description:
+      256-bit aes key. Only Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_ppk0hash@a0$":
+    type: object
+    description:
+      384-bit PPK0 hash. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_ppk1hash@d0$":
+    type: object
+    description:
+      384-bit PPK1 hash. Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+  "^efuse_pufuser@100$":
+    type: object
+    description:
+      This node represents the 127(0x7F) 32-bit PUF(Physical Unclonable Function)
+      helper data efuses which are repurposed as user fuses.
+      Read and Write is supported.
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
 additionalProperties: false
 
 examples:
@@ -56,4 +190,81 @@ examples:
         soc_revision: soc_revision@0 {
             reg = <0x0 0x4>;
         };
+        /*
+        * efuse memory access:
+        * all the efuse fields need to be read
+        * with the exact size specified in the node
+        */
+        /* DNA */
+        efuse_dna: efuse_dna@c {
+            reg = <0xc 0xc>;
+        };
+        /* User 0 */
+        efuse_usr0: efuse_usr0@20 {
+            reg = <0x20 0x4>;
+        };
+        /* User 1 */
+        efuse_usr1: efuse_usr1@24 {
+            reg = <0x24 0x4>;
+        };
+        /* User 2 */
+        efuse_usr2: efuse_usr2@28 {
+            reg = <0x28 0x4>;
+        };
+        /* User 3 */
+        efuse_usr3: efuse_usr3@2c {
+            reg = <0x2c 0x4>;
+        };
+        /* User 4 */
+        efuse_usr4: efuse_usr4@30 {
+            reg = <0x30 0x4>;
+        };
+        /* User 5 */
+        efuse_usr5: efuse_usr5@34 {
+            reg = <0x34 0x4>;
+        };
+        /* User 6 */
+        efuse_usr6: efuse_usr6@38 {
+            reg = <0x38 0x4>;
+        };
+        /* User 7 */
+        efuse_usr7: efuse_usr7@3c {
+            reg = <0x3c 0x4>;
+        };
+        /* Misc user control bits */
+        efuse_miscusr: efuse_miscusr@40 {
+            reg = <0x40 0x4>;
+        };
+        /* PUF chash */
+        efuse_chash: efuse_chash@50 {
+            reg = <0x50 0x4>;
+        };
+        /* PUF misc */
+        efuse_pufmisc: efuse_pufmisc@54 {
+            reg = <0x54 0x4>;
+        };
+        /* SEC_CTRL */
+        efuse_sec: efuse_sec@58 {
+            reg = <0x58 0x4>;
+        };
+        /* SPK ID */
+        efuse_spkid: efuse_spkid@5c {
+            reg = <0x5c 0x4>;
+        };
+        /* AES Key */
+        efuse_aeskey: efuse_aeskey@60 {
+            reg = <0x60 0x20>;
+        };
+        /* PPK0 hash */
+        efuse_ppk0hash: efuse_ppk0hash@a0 {
+            reg = <0xa0 0x30>;
+        };
+        /* PPK1 hash */
+        efuse_ppk1hash: efuse_ppk1hash@d0 {
+            reg = <0xd0 0x30>;
+        };
+        /* PUF user fuses */
+        efuse_pufuser: efuse_pufuser@100  {
+            reg = <0x100 0x7F>;
+        };
     };
-- 
2.36.1

[PATCH 4/5] arm64: zynqmp: Add ZynqnMP nvmem nodes

[Praveen Teja Kundanala]

Add nvmem DT nodes for ZynqMP SOC

Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 55 ++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 02cfcc716936..b8807dcce442 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -190,6 +190,61 @@ nvmem_firmware {
 				soc_revision: soc_revision@0 {
 					reg = <0x0 0x4>;
 				};
+				/* efuse access */
+				efuse_dna: efuse_dna@c {
+					reg = <0xc 0xc>;
+				};
+				efuse_usr0: efuse_usr0@20 {
+					reg = <0x20 0x4>;
+				};
+				efuse_usr1: efuse_usr1@24 {
+					reg = <0x24 0x4>;
+				};
+				efuse_usr2: efuse_usr2@28 {
+					reg = <0x28 0x4>;
+				};
+				efuse_usr3: efuse_usr3@2c {
+					reg = <0x2c 0x4>;
+				};
+				efuse_usr4: efuse_usr4@30 {
+					reg = <0x30 0x4>;
+				};
+				efuse_usr5: efuse_usr5@34 {
+					reg = <0x34 0x4>;
+				};
+				efuse_usr6: efuse_usr6@38 {
+					reg = <0x38 0x4>;
+				};
+				efuse_usr7: efuse_usr7@3c {
+					reg = <0x3c 0x4>;
+				};
+				efuse_miscusr: efuse_miscusr@40 {
+					reg = <0x40 0x4>;
+				};
+				efuse_chash: efuse_chash@50 {
+					reg = <0x50 0x4>;
+				};
+				efuse_pufmisc: efuse_pufmisc@54 {
+					reg = <0x54 0x4>;
+				};
+				efuse_sec: efuse_sec@58 {
+					reg = <0x58 0x4>;
+				};
+				efuse_spkid: efuse_spkid@5c {
+					reg = <0x5c 0x4>;
+				};
+				efuse_aeskey: efuse_aeskey@60 {
+					reg = <0x60 0x20>;
+				};
+				efuse_ppk0hash: efuse_ppk0hash@a0 {
+					reg = <0xa0 0x30>;
+				};
+				efuse_ppk1hash: efuse_ppk1hash@d0 {
+					reg = <0xd0 0x30>;
+				};
+				efuse_pufuser: efuse_pufuser@100 {
+					reg = <0x100 0x7F>;
+				};
 			};
 
 			zynqmp_pcap: pcap {
-- 
2.36.1

[PATCH 5/5] nvmem: zynqmp_nvmem: Add support to access efuse

[Praveen Teja Kundanala]

Add support to read/write efuse memory map of ZynqMP
Below are the offsets of ZynqMP efuse memory map
	0 - SOC version(read only)
	0xC - 0xFC -ZynqMP specific purpose efuses
	0x100 - 0x17F - Physical Unclonable Function(PUF)
                efuses repurposed as user efuses

Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
 drivers/nvmem/zynqmp_nvmem.c | 216 ++++++++++++++++++++++++++++++-----
 1 file changed, 185 insertions(+), 31 deletions(-)

diff --git a/drivers/nvmem/zynqmp_nvmem.c b/drivers/nvmem/zynqmp_nvmem.c
index f49bb9a26d05..e6123a32268a 100644
--- a/drivers/nvmem/zynqmp_nvmem.c
+++ b/drivers/nvmem/zynqmp_nvmem.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright (C) 2019 Xilinx, Inc.
+ * Copyright (C), 2022 - 2023 Advanced Micro Devices, Inc.
  */
 
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
 #include <linux/of.h>
@@ -10,36 +12,190 @@
 #include <linux/firmware/xlnx-zynqmp.h>
 
 #define SILICON_REVISION_MASK 0xF
+#define P_USER_0_64_UPPER_MASK	GENMASK(31, 16)
+#define P_USER_127_LOWER_4_BIT_MASK GENMASK(3, 0)
+#define WORD_INBYTES		4
+#define SOC_VER_SIZE		0x4
+#define EFUSE_MEMORY_SIZE	0x177
+#define UNUSED_SPACE		0x8
+#define ZYNQMP_NVMEM_SIZE	(SOC_VER_SIZE + UNUSED_SPACE + \
+				 EFUSE_MEMORY_SIZE)
+#define SOC_VERSION_OFFSET	0x0
+#define EFUSE_START_OFFSET	0xC
+#define EFUSE_END_OFFSET	0xFC
+#define EFUSE_PUF_START_OFFSET	0x100
+#define EFUSE_PUF_MID_OFFSET	0x140
+#define EFUSE_PUF_END_OFFSET	0x17F
+#define EFUSE_NOT_ENABLED	29
 
-struct zynqmp_nvmem_data {
-	struct device *dev;
-	struct nvmem_device *nvmem;
+/*
+ * efuse access type
+ */
+enum efuse_access {
+	EFUSE_READ = 0,
+	EFUSE_WRITE
+};
+
+/**
+ * struct xilinx_efuse - the basic structure
+ * @src:	address of the buffer to store the data to be write/read
+ * @size:	read/write word count
+ * @offset:	read/write offset
+ * @flag:	0 - represents efuse read and 1- represents efuse write
+ * @pufuserfuse:0 - represents non-puf efuses, offset is used for read/write
+ *		1 - represents puf user fuse row number.
+ *
+ * this structure stores all the required details to
+ * read/write efuse memory.
+ */
+struct xilinx_efuse {
+	u64 src;
+	u32 size;
+	u32 offset;
+	enum efuse_access flag;
+	u32 pufuserfuse;
 };
 
-static int zynqmp_nvmem_read(void *context, unsigned int offset,
-			     void *val, size_t bytes)
+static int zynqmp_efuse_access(void *context, unsigned int offset,
+			       void *val, size_t bytes, enum efuse_access flag,
+			       unsigned int pufflag)
 {
+	struct device *dev = context;
+	struct xilinx_efuse *efuse;
+	dma_addr_t dma_addr;
+	dma_addr_t dma_buf;
+	size_t words = bytes / WORD_INBYTES;
 	int ret;
-	int idcode, version;
-	struct zynqmp_nvmem_data *priv = context;
-
-	ret = zynqmp_pm_get_chipid(&idcode, &version);
-	if (ret < 0)
-		return ret;
+	int value;
+	char *data;
+
+	if (bytes % WORD_INBYTES != 0) {
+		dev_err(dev, "Bytes requested should be word aligned\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (pufflag == 0 && offset % WORD_INBYTES) {
+		dev_err(dev, "Offset requested should be word aligned\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (pufflag == 1 && flag == EFUSE_WRITE) {
+		memcpy(&value, val, bytes);
+		if ((offset == EFUSE_PUF_START_OFFSET ||
+		     offset == EFUSE_PUF_MID_OFFSET) &&
+		    value & P_USER_0_64_UPPER_MASK) {
+			dev_err(dev, "Only lower 4 bytes are allowed to be programmed in P_USER_0 & P_USER_64\n");
+			return -EOPNOTSUPP;
+		}
+
+		if (offset == EFUSE_PUF_END_OFFSET &&
+		    (value & P_USER_127_LOWER_4_BIT_MASK)) {
+			dev_err(dev, "Only MSB 28 bits are allowed to be programmed for P_USER_127\n");
+			return -EOPNOTSUPP;
+		}
+	}
+
+	efuse = dma_alloc_coherent(dev, sizeof(struct xilinx_efuse),
+				   &dma_addr, GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
 
-	dev_dbg(priv->dev, "Read chipid val %x %x\n", idcode, version);
-	*(int *)val = version & SILICON_REVISION_MASK;
+	data = dma_alloc_coherent(dev, sizeof(bytes),
+				  &dma_buf, GFP_KERNEL);
+	if (!data) {
+		ret = -ENOMEM;
+		goto efuse_data_fail;
+	}
+
+	if (flag == EFUSE_WRITE) {
+		memcpy(data, val, bytes);
+		efuse->flag = EFUSE_WRITE;
+	} else {
+		efuse->flag = EFUSE_READ;
+	}
+
+	efuse->src = dma_buf;
+	efuse->size = words;
+	efuse->offset = offset;
+	efuse->pufuserfuse = pufflag;
+
+	zynqmp_pm_efuse_access(dma_addr, (u32 *)&ret);
+	if (ret != 0) {
+		if (ret == EFUSE_NOT_ENABLED) {
+			dev_err(dev, "efuse access is not enabled\n");
+			ret = -EOPNOTSUPP;
+		} else {
+			dev_err(dev, "Error in efuse read %x\n", ret);
+			ret = -EPERM;
+		}
+		goto efuse_access_err;
+	}
+
+	if (flag == EFUSE_READ)
+		memcpy(val, data, bytes);
+efuse_access_err:
+	dma_free_coherent(dev, sizeof(bytes),
+			  data, dma_buf);
+efuse_data_fail:
+	dma_free_coherent(dev, sizeof(struct xilinx_efuse),
+			  efuse, dma_addr);
+
+	return ret;
+}
 
-	return 0;
+static int zynqmp_nvmem_read(void *context, unsigned int offset, void *val, size_t bytes)
+{
+	struct device *dev = context;
+	int ret;
+	int pufflag = 0;
+	int idcode;
+	int version;
+
+	if (offset >= EFUSE_PUF_START_OFFSET && offset <= EFUSE_PUF_END_OFFSET)
+		pufflag = 1;
+
+	switch (offset) {
+	/* Soc version offset is zero */
+	case SOC_VERSION_OFFSET:
+		if (bytes != SOC_VER_SIZE)
+			return -EOPNOTSUPP;
+
+		ret = zynqmp_pm_get_chipid((u32 *)&idcode, (u32 *)&version);
+		if (ret < 0)
+			return ret;
+
+		dev_dbg(dev, "Read chipid val %x %x\n", idcode, version);
+		*(int *)val = version & SILICON_REVISION_MASK;
+		break;
+	/* Efuse offset starts from 0xc */
+	case EFUSE_START_OFFSET ... EFUSE_END_OFFSET:
+	case EFUSE_PUF_START_OFFSET ... EFUSE_PUF_END_OFFSET:
+		ret = zynqmp_efuse_access(context, offset, val,
+					  bytes, EFUSE_READ, pufflag);
+		break;
+	default:
+		*(u32 *)val = 0xDEADBEEF;
+		ret = 0;
+		break;
+	}
+
+	return ret;
 }
 
-static struct nvmem_config econfig = {
-	.name = "zynqmp-nvmem",
-	.owner = THIS_MODULE,
-	.word_size = 1,
-	.size = 1,
-	.read_only = true,
-};
+static int zynqmp_nvmem_write(void *context,
+			      unsigned int offset, void *val, size_t bytes)
+{
+	int pufflag = 0;
+
+	if (offset < EFUSE_START_OFFSET || offset > EFUSE_PUF_END_OFFSET)
+		return -EOPNOTSUPP;
+
+	if (offset >= EFUSE_PUF_START_OFFSET && offset <= EFUSE_PUF_END_OFFSET)
+		pufflag = 1;
+
+	return zynqmp_efuse_access(context, offset,
+				   val, bytes, EFUSE_WRITE, pufflag);
+}
 
 static const struct of_device_id zynqmp_nvmem_match[] = {
 	{ .compatible = "xlnx,zynqmp-nvmem-fw", },
@@ -50,20 +206,18 @@ MODULE_DEVICE_TABLE(of, zynqmp_nvmem_match);
 static int zynqmp_nvmem_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct zynqmp_nvmem_data *priv;
-
-	priv = devm_kzalloc(dev, sizeof(struct zynqmp_nvmem_data), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	struct nvmem_config econfig = {};
 
-	priv->dev = dev;
+	econfig.name = "zynqmp-nvmem";
+	econfig.owner = THIS_MODULE;
+	econfig.word_size = 1;
+	econfig.size = ZYNQMP_NVMEM_SIZE;
 	econfig.dev = dev;
+	econfig.priv = dev;
 	econfig.reg_read = zynqmp_nvmem_read;
-	econfig.priv = priv;
-
-	priv->nvmem = devm_nvmem_register(dev, &econfig);
+	econfig.reg_write = zynqmp_nvmem_write;
 
-	return PTR_ERR_OR_ZERO(priv->nvmem);
+	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &econfig));
 }
 
 static struct platform_driver zynqmp_nvmem_driver = {
-- 
2.36.1

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Krzysztof Kozlowski]

On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
> 
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      | 46 ---------------
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 59 +++++++++++++++++++
>  2 files changed, 59 insertions(+), 46 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>  create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> deleted file mode 100644
> index 4881561b3a02..000000000000
> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> +++ /dev/null
> @@ -1,46 +0,0 @@
> ---------------------------------------------------------------------------
> -=  Zynq UltraScale+ MPSoC nvmem firmware driver binding =
> ---------------------------------------------------------------------------
> -The nvmem_firmware node provides access to the hardware related data
> -like soc revision, IDCODE... etc, By using the firmware interface.
> -
> -Required properties:
> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
> -
> -= Data cells =
> -Are child nodes of silicon id, bindings of which as described in
> -bindings/nvmem/nvmem.txt
> -
> --------
> - Example
> --------
> -firmware {
> -	zynqmp_firmware: zynqmp-firmware {
> -		compatible = "xlnx,zynqmp-firmware";
> -		method = "smc";
> -
> -		nvmem_firmware {
> -			compatible = "xlnx,zynqmp-nvmem-fw";
> -			#address-cells = <1>;
> -			#size-cells = <1>;
> -
> -			/* Data cells */
> -			soc_revision: soc_revision {
> -				reg = <0x0 0x4>;
> -			};
> -		};
> -	};
> -};
> -
> -= Data consumers =
> -Are device nodes which consume nvmem data cells.
> -
> -For example:
> -	pcap {
> -		...
> -
> -		nvmem-cells = <&soc_revision>;
> -		nvmem-cell-names = "soc_revision";
> -
> -		...
> -	};
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> new file mode 100644
> index 000000000000..e03ed8c32537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
> +
> +description: |
> +    The ZynqMP MPSoC provides access to the hardware related data
> +    like SOC revision, IDCODE.
> +
> +maintainers:
> +  - Kalyani Akula <kalyani.akula@amd.com>
> +  - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> +
> +allOf:
> +  - $ref: "nvmem.yaml#"

Drop quotes.

> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-nvmem-fw
> +
> +  '#address-cells':
> +    const: 1

Drop

> +
> +  '#size-cells':
> +    const: 1

Drop

> +
> +required:
> +  - compatible

required: block goes after patternProperties: block

> +
> +patternProperties:
> +  "^soc_revision@0$":

Why do you define individual memory cells? Is this part of a binding?
IOW, OS/Linux requires this?

> +    type: object
> +    description:
> +      This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false

Instead: unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    nvmem_firmware {

Underscores are not allowed in node names.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Look at other files for examples.


> +        compatible = "xlnx,zynqmp-nvmem-fw";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        /* Data cells */
> +        soc_revision: soc_revision@0 {

Drop unused label. No underscores in node names.


Best regards,
Krzysztof

Re: [PATCH 3/5] dt-bindings: nvmem: Add nodes for ZynqMP efuses

[Krzysztof Kozlowski]

On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
> Added nodes for ZynqMP specific purpose and PUF user efuses

Why?

> 
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 213 +++++++++++++++++-
>  1 file changed, 212 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> index e03ed8c32537..d2a036a80cda 100644
> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> @@ -8,7 +8,7 @@ title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
>  
>  description: |
>      The ZynqMP MPSoC provides access to the hardware related data
> -    like SOC revision, IDCODE.
> +    like SOC revision, IDCODE and specific purpose efuses.
>  
>  maintainers:
>    - Kalyani Akula <kalyani.akula@amd.com>
> @@ -43,6 +43,140 @@ patternProperties:
>      required:
>        - reg
>  
> +  "^efuse_dna@c$":

No, no underscores in node names. From where did you get this pattern?
Which upstream code has it?

All this and further parts are questionable. I don't understand why do
you do it.

Best regards,
Krzysztof

Re: [PATCH 4/5] arm64: zynqmp: Add ZynqnMP nvmem nodes

[Krzysztof Kozlowski]

On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
> Add nvmem DT nodes for ZynqMP SOC
> 
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 55 ++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index 02cfcc716936..b8807dcce442 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -190,6 +190,61 @@ nvmem_firmware {
>  				soc_revision: soc_revision@0 {
>  					reg = <0x0 0x4>;
>  				};
> +				/* efuse access */
> +				efuse_dna: efuse_dna@c {

No underscores in node names. I see now from where did you get the
initial pattern. Would be great if you fixed Xilinx DTS :/

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Rob Herring]

On Fri, 13 Oct 2023 15:44:47 +0530, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
> 
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      | 46 ---------------
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 59 +++++++++++++++++++
>  2 files changed, 59 insertions(+), 46 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>  create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml:18:11: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231013101450.573-3-praveen.teja.kundanala@amd.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 4/5] arm64: zynqmp: Add ZynqnMP nvmem nodes

[Michal Simek]

On 10/13/23 12:32, Krzysztof Kozlowski wrote:
> On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
>> Add nvmem DT nodes for ZynqMP SOC
>>
>> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
>> ---
>>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 55 ++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> index 02cfcc716936..b8807dcce442 100644
>> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> @@ -190,6 +190,61 @@ nvmem_firmware {
>>   				soc_revision: soc_revision@0 {
>>   					reg = <0x0 0x4>;
>>   				};
>> +				/* efuse access */
>> +				efuse_dna: efuse_dna@c {
> 
> No underscores in node names. I see now from where did you get the
> initial pattern. Would be great if you fixed Xilinx DTS :/

I actually fixed soc-revision here.

https://lore.kernel.org/all/5137958580c85a35cf6aadd1c33a2f6bcf81a9e5.1695040866.git.michal.simek@amd.com/

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 12:30, Krzysztof Kozlowski wrote:
> On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
>> Convert the xlnx,zynqmp-nvmem.txt to yaml.
>>
>> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
>> ---
>>   .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      | 46 ---------------
>>   .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 59 +++++++++++++++++++
>>   2 files changed, 59 insertions(+), 46 deletions(-)
>>   delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>>   create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>> deleted file mode 100644
>> index 4881561b3a02..000000000000
>> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>> +++ /dev/null
>> @@ -1,46 +0,0 @@
>> ---------------------------------------------------------------------------
>> -=  Zynq UltraScale+ MPSoC nvmem firmware driver binding =
>> ---------------------------------------------------------------------------
>> -The nvmem_firmware node provides access to the hardware related data
>> -like soc revision, IDCODE... etc, By using the firmware interface.
>> -
>> -Required properties:
>> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
>> -
>> -= Data cells =
>> -Are child nodes of silicon id, bindings of which as described in
>> -bindings/nvmem/nvmem.txt
>> -
>> --------
>> - Example
>> --------
>> -firmware {
>> -	zynqmp_firmware: zynqmp-firmware {
>> -		compatible = "xlnx,zynqmp-firmware";
>> -		method = "smc";
>> -
>> -		nvmem_firmware {
>> -			compatible = "xlnx,zynqmp-nvmem-fw";
>> -			#address-cells = <1>;
>> -			#size-cells = <1>;
>> -
>> -			/* Data cells */
>> -			soc_revision: soc_revision {
>> -				reg = <0x0 0x4>;
>> -			};
>> -		};
>> -	};
>> -};
>> -
>> -= Data consumers =
>> -Are device nodes which consume nvmem data cells.
>> -
>> -For example:
>> -	pcap {
>> -		...
>> -
>> -		nvmem-cells = <&soc_revision>;
>> -		nvmem-cell-names = "soc_revision";
>> -
>> -		...
>> -	};
>> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>> new file mode 100644
>> index 000000000000..e03ed8c32537
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>> @@ -0,0 +1,59 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
>> +
>> +description: |
>> +    The ZynqMP MPSoC provides access to the hardware related data
>> +    like SOC revision, IDCODE.
>> +
>> +maintainers:
>> +  - Kalyani Akula <kalyani.akula@amd.com>
>> +  - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
>> +
>> +allOf:
>> +  - $ref: "nvmem.yaml#"
> 
> Drop quotes.
> 
>> +
>> +properties:
>> +  compatible:
>> +    const: xlnx,zynqmp-nvmem-fw
>> +
>> +  '#address-cells':
>> +    const: 1
> 
> Drop
> 
>> +
>> +  '#size-cells':
>> +    const: 1
> 
> Drop
> 
>> +
>> +required:
>> +  - compatible
> 
> required: block goes after patternProperties: block
> 
>> +
>> +patternProperties:
>> +  "^soc_revision@0$":
> 
> Why do you define individual memory cells? Is this part of a binding?
> IOW, OS/Linux requires this?

nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() 
calls. It means you should be able to describe internal layout that's why names 
are used. And address in name is there because of reg property is used to 
describe base offset and size.

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Krzysztof Kozlowski]

On 13/10/2023 13:22, Michal Simek wrote:
>>
>>> +
>>> +required:
>>> +  - compatible
>>
>> required: block goes after patternProperties: block
>>
>>> +
>>> +patternProperties:
>>> +  "^soc_revision@0$":
>>
>> Why do you define individual memory cells? Is this part of a binding?
>> IOW, OS/Linux requires this?
> 
> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() 
> calls. It means you should be able to describe internal layout that's why names 
> are used. And address in name is there because of reg property is used to 
> describe base offset and size.

That's not really what I am asking. Why internal layout of memory must
be part of the bindings?

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 13:46, Krzysztof Kozlowski wrote:
> On 13/10/2023 13:22, Michal Simek wrote:
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>
>>> required: block goes after patternProperties: block
>>>
>>>> +
>>>> +patternProperties:
>>>> +  "^soc_revision@0$":
>>>
>>> Why do you define individual memory cells? Is this part of a binding?
>>> IOW, OS/Linux requires this?
>>
>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>> calls. It means you should be able to describe internal layout that's why names
>> are used. And address in name is there because of reg property is used to
>> describe base offset and size.
> 
> That's not really what I am asking. Why internal layout of memory must
> be part of the bindings?

It doesn't need to be but offsets are hardcoded inside the driver itself and 
they can't be different.  On different nvmem locations like MAC location in 
eeprom this can vary across boards but in this case location has to be only like 
this.
I am fine if they don't need to be actually check but there is no any other way 
how they can be composed. And also others are not valid that's why not to 
describe only valid one.

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Krzysztof Kozlowski]

On 13/10/2023 13:51, Michal Simek wrote:
> 
> 
> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>
>>>> required: block goes after patternProperties: block
>>>>
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^soc_revision@0$":
>>>>
>>>> Why do you define individual memory cells? Is this part of a binding?
>>>> IOW, OS/Linux requires this?
>>>
>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>> calls. It means you should be able to describe internal layout that's why names
>>> are used. And address in name is there because of reg property is used to
>>> describe base offset and size.
>>
>> That's not really what I am asking. Why internal layout of memory must
>> be part of the bindings?
> 
> It doesn't need to be but offsets are hardcoded inside the driver itself and 
> they can't be different.

Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
hard-coded offsets.

>  On different nvmem locations like MAC location in 
> eeprom this can vary across boards but in this case location has to be only like 
> this.
> I am fine if they don't need to be actually check but there is no any other way 
> how they can be composed. And also others are not valid that's why not to 
> describe only valid one.

OK, that would be valid (if I find anywhere the offsets) and answers my
questions but I wish it was documented somewhere. Because now you are
making it a binding, so it cannot change (e.g. for different devices
with same hardware but different firmware or manufacturing process, for
future hardware sharing this binding).

In any case the binding should have only items which are really fixed
and OS depends on them. Neither this nor next commit answers this.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 13:58, Krzysztof Kozlowski wrote:
> On 13/10/2023 13:51, Michal Simek wrote:
>>
>>
>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>
>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>
>>>>> required: block goes after patternProperties: block
>>>>>
>>>>>> +
>>>>>> +patternProperties:
>>>>>> +  "^soc_revision@0$":
>>>>>
>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>> IOW, OS/Linux requires this?
>>>>
>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>> calls. It means you should be able to describe internal layout that's why names
>>>> are used. And address in name is there because of reg property is used to
>>>> describe base offset and size.
>>>
>>> That's not really what I am asking. Why internal layout of memory must
>>> be part of the bindings?
>>
>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>> they can't be different.
> 
> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
> hard-coded offsets.

Current driver supports only soc revision from offset 0.
But if you look at 5/5 you need to define offsets where information is present.
+#define SOC_VERSION_OFFSET	0x0
+#define EFUSE_START_OFFSET	0xC
+#define EFUSE_END_OFFSET	0xFC
+#define EFUSE_PUF_START_OFFSET	0x100
+#define EFUSE_PUF_MID_OFFSET	0x140
+#define EFUSE_PUF_END_OFFSET	0x17F


> 
>>   On different nvmem locations like MAC location in
>> eeprom this can vary across boards but in this case location has to be only like
>> this.
>> I am fine if they don't need to be actually check but there is no any other way
>> how they can be composed. And also others are not valid that's why not to
>> describe only valid one.
> 
> OK, that would be valid (if I find anywhere the offsets) and answers my
> questions but I wish it was documented somewhere. Because now you are
> making it a binding, so it cannot change (e.g. for different devices
> with same hardware but different firmware or manufacturing process, for
> future hardware sharing this binding).

ZynqMP firmware with xlnx,zynqmp-nvmem-fw compatible string is using this map.
If there is different firmware likely xlnx,zynqmp-nvmem-fw compatible string 
can't be used.

> In any case the binding should have only items which are really fixed
> and OS depends on them. Neither this nor next commit answers this.

Actually 5/5 has this in commit message
	0 - SOC version(read only)
	0xC - 0xFC -ZynqMP specific purpose efuses
	0x100 - 0x17F - Physical Unclonable Function(PUF)
                 efuses repurposed as user efuses

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Krzysztof Kozlowski]

On 13/10/2023 14:08, Michal Simek wrote:
> 
> 
> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
>> On 13/10/2023 13:51, Michal Simek wrote:
>>>
>>>
>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>>
>>>>>>> +
>>>>>>> +required:
>>>>>>> +  - compatible
>>>>>>
>>>>>> required: block goes after patternProperties: block
>>>>>>
>>>>>>> +
>>>>>>> +patternProperties:
>>>>>>> +  "^soc_revision@0$":
>>>>>>
>>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>>> IOW, OS/Linux requires this?
>>>>>
>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>>> calls. It means you should be able to describe internal layout that's why names
>>>>> are used. And address in name is there because of reg property is used to
>>>>> describe base offset and size.
>>>>
>>>> That's not really what I am asking. Why internal layout of memory must
>>>> be part of the bindings?
>>>
>>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>>> they can't be different.
>>
>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
>> hard-coded offsets.
> 
> Current driver supports only soc revision from offset 0.
> But if you look at 5/5 you need to define offsets where information is present.
> +#define SOC_VERSION_OFFSET	0x0
> +#define EFUSE_START_OFFSET	0xC
> +#define EFUSE_END_OFFSET	0xFC
> +#define EFUSE_PUF_START_OFFSET	0x100
> +#define EFUSE_PUF_MID_OFFSET	0x140
> +#define EFUSE_PUF_END_OFFSET	0x17F

There is nothing like this in existing driver, so the argument that "I
am adding this to the binding during conversion because driver needs it"
is not true. Conversion is only a conversion.

Now, if you want to add something new to the binding because of new
driver changes, that's separate topic.

And since it is new change in the driver I can comment: please don't.
Your nvmem driver should not depend on it. nvmem is only the provider.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 14:54, Krzysztof Kozlowski wrote:
> On 13/10/2023 14:08, Michal Simek wrote:
>>
>>
>> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
>>> On 13/10/2023 13:51, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>>>
>>>>>>>> +
>>>>>>>> +required:
>>>>>>>> +  - compatible
>>>>>>>
>>>>>>> required: block goes after patternProperties: block
>>>>>>>
>>>>>>>> +
>>>>>>>> +patternProperties:
>>>>>>>> +  "^soc_revision@0$":
>>>>>>>
>>>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>>>> IOW, OS/Linux requires this?
>>>>>>
>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>>>> calls. It means you should be able to describe internal layout that's why names
>>>>>> are used. And address in name is there because of reg property is used to
>>>>>> describe base offset and size.
>>>>>
>>>>> That's not really what I am asking. Why internal layout of memory must
>>>>> be part of the bindings?
>>>>
>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>>>> they can't be different.
>>>
>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
>>> hard-coded offsets.
>>
>> Current driver supports only soc revision from offset 0.
>> But if you look at 5/5 you need to define offsets where information is present.
>> +#define SOC_VERSION_OFFSET	0x0
>> +#define EFUSE_START_OFFSET	0xC
>> +#define EFUSE_END_OFFSET	0xFC
>> +#define EFUSE_PUF_START_OFFSET	0x100
>> +#define EFUSE_PUF_MID_OFFSET	0x140
>> +#define EFUSE_PUF_END_OFFSET	0x17F
> 
> There is nothing like this in existing driver, so the argument that "I
> am adding this to the binding during conversion because driver needs it"
> is not true. Conversion is only a conversion.

Conversion in 2/5 is adding only soc revision which is already there. It is 
starting from 0 and world size is 1. And 0 is not listed because that's start 
all the time.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39

And soc revision was also listed in origin binding example.

> Now, if you want to add something new to the binding because of new
> driver changes, that's separate topic.

Functionality in firmware is there for quite a long time but as I said I am fine 
if map is not going to be inside dt binding spec.

> And since it is new change in the driver I can comment: please don't.
> Your nvmem driver should not depend on it. nvmem is only the provider.

Let's see what Srinivas says about implementation. If driver should be just 
provider then pretty much current driver should be completely rewritten to 
different style. I mean to have just transport via SMCs with offset/size and 
then providing functionality in firmware.

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Krzysztof Kozlowski]

On 13/10/2023 15:06, Michal Simek wrote:
> 
> 
> On 10/13/23 14:54, Krzysztof Kozlowski wrote:
>> On 13/10/2023 14:08, Michal Simek wrote:
>>>
>>>
>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
>>>> On 13/10/2023 13:51, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>>>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +required:
>>>>>>>>> +  - compatible
>>>>>>>>
>>>>>>>> required: block goes after patternProperties: block
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +patternProperties:
>>>>>>>>> +  "^soc_revision@0$":
>>>>>>>>
>>>>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>>>>> IOW, OS/Linux requires this?
>>>>>>>
>>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>>>>> calls. It means you should be able to describe internal layout that's why names
>>>>>>> are used. And address in name is there because of reg property is used to
>>>>>>> describe base offset and size.
>>>>>>
>>>>>> That's not really what I am asking. Why internal layout of memory must
>>>>>> be part of the bindings?
>>>>>
>>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>>>>> they can't be different.
>>>>
>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
>>>> hard-coded offsets.
>>>
>>> Current driver supports only soc revision from offset 0.
>>> But if you look at 5/5 you need to define offsets where information is present.
>>> +#define SOC_VERSION_OFFSET	0x0
>>> +#define EFUSE_START_OFFSET	0xC
>>> +#define EFUSE_END_OFFSET	0xFC
>>> +#define EFUSE_PUF_START_OFFSET	0x100
>>> +#define EFUSE_PUF_MID_OFFSET	0x140
>>> +#define EFUSE_PUF_END_OFFSET	0x17F
>>
>> There is nothing like this in existing driver, so the argument that "I
>> am adding this to the binding during conversion because driver needs it"
>> is not true. Conversion is only a conversion.
> 
> Conversion in 2/5 is adding only soc revision which is already there. It is 
> starting from 0 and world size is 1. And 0 is not listed because that's start 
> all the time.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39

This defines the nvmem config, not what should be where.

> 
> And soc revision was also listed in origin binding example.

Example is not a binding. Please drop enforcement of some specific nodes
from the binding.

> 
>> Now, if you want to add something new to the binding because of new
>> driver changes, that's separate topic.
> 
> Functionality in firmware is there for quite a long time but as I said I am fine 
> if map is not going to be inside dt binding spec.
> 
>> And since it is new change in the driver I can comment: please don't.
>> Your nvmem driver should not depend on it. nvmem is only the provider.
> 
> Let's see what Srinivas says about implementation. If driver should be just 
> provider then pretty much current driver should be completely rewritten to 
> different style. I mean to have just transport via SMCs with offset/size and 
> then providing functionality in firmware.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 15:10, Krzysztof Kozlowski wrote:
> On 13/10/2023 15:06, Michal Simek wrote:
>>
>>
>> On 10/13/23 14:54, Krzysztof Kozlowski wrote:
>>> On 13/10/2023 14:08, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
>>>>> On 13/10/2023 13:51, Michal Simek wrote:
>>>>>>
>>>>>>
>>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>>>>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +required:
>>>>>>>>>> +  - compatible
>>>>>>>>>
>>>>>>>>> required: block goes after patternProperties: block
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +patternProperties:
>>>>>>>>>> +  "^soc_revision@0$":
>>>>>>>>>
>>>>>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>>>>>> IOW, OS/Linux requires this?
>>>>>>>>
>>>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>>>>>> calls. It means you should be able to describe internal layout that's why names
>>>>>>>> are used. And address in name is there because of reg property is used to
>>>>>>>> describe base offset and size.
>>>>>>>
>>>>>>> That's not really what I am asking. Why internal layout of memory must
>>>>>>> be part of the bindings?
>>>>>>
>>>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>>>>>> they can't be different.
>>>>>
>>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
>>>>> hard-coded offsets.
>>>>
>>>> Current driver supports only soc revision from offset 0.
>>>> But if you look at 5/5 you need to define offsets where information is present.
>>>> +#define SOC_VERSION_OFFSET	0x0
>>>> +#define EFUSE_START_OFFSET	0xC
>>>> +#define EFUSE_END_OFFSET	0xFC
>>>> +#define EFUSE_PUF_START_OFFSET	0x100
>>>> +#define EFUSE_PUF_MID_OFFSET	0x140
>>>> +#define EFUSE_PUF_END_OFFSET	0x17F
>>>
>>> There is nothing like this in existing driver, so the argument that "I
>>> am adding this to the binding during conversion because driver needs it"
>>> is not true. Conversion is only a conversion.
>>
>> Conversion in 2/5 is adding only soc revision which is already there. It is
>> starting from 0 and world size is 1. And 0 is not listed because that's start
>> all the time.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39
> 
> This defines the nvmem config, not what should be where.

If you have only one entry you are also saying where it is.

Thanks,
Michal

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Rob Herring]

On Fri, Oct 13, 2023 at 03:44:47PM +0530, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
> 
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.txt      | 46 ---------------
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml     | 59 +++++++++++++++++++
>  2 files changed, 59 insertions(+), 46 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>  create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> deleted file mode 100644
> index 4881561b3a02..000000000000
> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> +++ /dev/null
> @@ -1,46 +0,0 @@
> ---------------------------------------------------------------------------
> -=  Zynq UltraScale+ MPSoC nvmem firmware driver binding =
> ---------------------------------------------------------------------------
> -The nvmem_firmware node provides access to the hardware related data
> -like soc revision, IDCODE... etc, By using the firmware interface.
> -
> -Required properties:
> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
> -
> -= Data cells =
> -Are child nodes of silicon id, bindings of which as described in
> -bindings/nvmem/nvmem.txt
> -
> --------
> - Example
> --------
> -firmware {
> -	zynqmp_firmware: zynqmp-firmware {
> -		compatible = "xlnx,zynqmp-firmware";
> -		method = "smc";
> -
> -		nvmem_firmware {
> -			compatible = "xlnx,zynqmp-nvmem-fw";
> -			#address-cells = <1>;
> -			#size-cells = <1>;
> -
> -			/* Data cells */
> -			soc_revision: soc_revision {
> -				reg = <0x0 0x4>;
> -			};
> -		};
> -	};
> -};
> -
> -= Data consumers =
> -Are device nodes which consume nvmem data cells.
> -
> -For example:
> -	pcap {
> -		...
> -
> -		nvmem-cells = <&soc_revision>;
> -		nvmem-cell-names = "soc_revision";
> -
> -		...
> -	};
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> new file mode 100644
> index 000000000000..e03ed8c32537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
> +
> +description: |
> +    The ZynqMP MPSoC provides access to the hardware related data
> +    like SOC revision, IDCODE.
> +
> +maintainers:
> +  - Kalyani Akula <kalyani.akula@amd.com>
> +  - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> +
> +allOf:
> +  - $ref: "nvmem.yaml#"
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-nvmem-fw
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +
> +patternProperties:
> +  "^soc_revision@0$":

Fixed string, not a pattern. dtschema will now flag this. Thanks for the 
test case. ;)

> +    type: object

Needs 'additionalProperties' or...


> +    description:
> +      This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
> +

> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - reg

fixed-cell.yaml (via nvmem.yaml) already says this, so this can all be 
dropped. Except that this[1] just landed, so you will need to adapt to 
it.

Though there is an issue that fixed-cell.yaml doesn't restrict the 
allowed properties, so that needs to happen somewhere... Not sure what 
the fix is. Hopefully fixed-cell.yaml can just be changed to 
'additionalProperties: false', but need to see if other properties are 
in use.

Rob

[1] https://lore.kernel.org/all/169667333484.74178.7121029453685069845.b4-ty@linaro.org/

Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Michal Simek]

On 10/13/23 15:10, Krzysztof Kozlowski wrote:
> On 13/10/2023 15:06, Michal Simek wrote:
>>
>>
>> On 10/13/23 14:54, Krzysztof Kozlowski wrote:
>>> On 13/10/2023 14:08, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
>>>>> On 13/10/2023 13:51, Michal Simek wrote:
>>>>>>
>>>>>>
>>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>>>>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +required:
>>>>>>>>>> +  - compatible
>>>>>>>>>
>>>>>>>>> required: block goes after patternProperties: block
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +patternProperties:
>>>>>>>>>> +  "^soc_revision@0$":
>>>>>>>>>
>>>>>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>>>>>> IOW, OS/Linux requires this?
>>>>>>>>
>>>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>>>>>> calls. It means you should be able to describe internal layout that's why names
>>>>>>>> are used. And address in name is there because of reg property is used to
>>>>>>>> describe base offset and size.
>>>>>>>
>>>>>>> That's not really what I am asking. Why internal layout of memory must
>>>>>>> be part of the bindings?
>>>>>>
>>>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>>>>>> they can't be different.
>>>>>
>>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
>>>>> hard-coded offsets.
>>>>
>>>> Current driver supports only soc revision from offset 0.
>>>> But if you look at 5/5 you need to define offsets where information is present.
>>>> +#define SOC_VERSION_OFFSET	0x0
>>>> +#define EFUSE_START_OFFSET	0xC
>>>> +#define EFUSE_END_OFFSET	0xFC
>>>> +#define EFUSE_PUF_START_OFFSET	0x100
>>>> +#define EFUSE_PUF_MID_OFFSET	0x140
>>>> +#define EFUSE_PUF_END_OFFSET	0x17F
>>>
>>> There is nothing like this in existing driver, so the argument that "I
>>> am adding this to the binding during conversion because driver needs it"
>>> is not true. Conversion is only a conversion.
>>
>> Conversion in 2/5 is adding only soc revision which is already there. It is
>> starting from 0 and world size is 1. And 0 is not listed because that's start
>> all the time.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39
> 
> This defines the nvmem config, not what should be where.
> 
>>
>> And soc revision was also listed in origin binding example.
> 
> Example is not a binding. Please drop enforcement of some specific nodes
> from the binding.

ok. Fine.
Praveen: Please drop that descriptions about sub nodes.

Thanks,
Michal

RE: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

[Kundanala, Praveen Teja]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Monday, October 16, 2023 1:32 PM
> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Kundanala, Praveen
> Teja <praveen.teja.kundanala@amd.com>; srinivas.kandagatla@linaro.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt
> to yaml
>
>
>
> On 10/13/23 15:10, Krzysztof Kozlowski wrote:
> > On 13/10/2023 15:06, Michal Simek wrote:
> >>
> >>
> >> On 10/13/23 14:54, Krzysztof Kozlowski wrote:
> >>> On 13/10/2023 14:08, Michal Simek wrote:
> >>>>
> >>>>
> >>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote:
> >>>>> On 13/10/2023 13:51, Michal Simek wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
> >>>>>>> On 13/10/2023 13:22, Michal Simek wrote:
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +required:
> >>>>>>>>>> +  - compatible
> >>>>>>>>>
> >>>>>>>>> required: block goes after patternProperties: block
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +patternProperties:
> >>>>>>>>>> +  "^soc_revision@0$":
> >>>>>>>>>
> >>>>>>>>> Why do you define individual memory cells? Is this part of a
> binding?
> >>>>>>>>> IOW, OS/Linux requires this?
> >>>>>>>>
> >>>>>>>> nvmem has in kernel interface where you can reference to nodes.
> >>>>>>>> nvmem_cell_get() calls. It means you should be able to describe
> >>>>>>>> internal layout that's why names are used. And address in name
> >>>>>>>> is there because of reg property is used to describe base offset and
> size.
> >>>>>>>
> >>>>>>> That's not really what I am asking. Why internal layout of
> >>>>>>> memory must be part of the bindings?
> >>>>>>
> >>>>>> It doesn't need to be but offsets are hardcoded inside the driver
> >>>>>> itself and they can't be different.
> >>>>>
> >>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not
> see
> >>>>> any hard-coded offsets.
> >>>>
> >>>> Current driver supports only soc revision from offset 0.
> >>>> But if you look at 5/5 you need to define offsets where information is
> present.
> >>>> +#define SOC_VERSION_OFFSET      0x0
> >>>> +#define EFUSE_START_OFFSET      0xC
> >>>> +#define EFUSE_END_OFFSET        0xFC
> >>>> +#define EFUSE_PUF_START_OFFSET  0x100
> >>>> +#define EFUSE_PUF_MID_OFFSET    0x140
> >>>> +#define EFUSE_PUF_END_OFFSET    0x17F
> >>>
> >>> There is nothing like this in existing driver, so the argument that
> >>> "I am adding this to the binding during conversion because driver needs it"
> >>> is not true. Conversion is only a conversion.
> >>
> >> Conversion in 2/5 is adding only soc revision which is already there.
> >> It is starting from 0 and world size is 1. And 0 is not listed
> >> because that's start all the time.
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr
> >> ee/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39
> >
> > This defines the nvmem config, not what should be where.
> >
> >>
> >> And soc revision was also listed in origin binding example.
> >
> > Example is not a binding. Please drop enforcement of some specific
> > nodes from the binding.
>
> ok. Fine.
> Praveen: Please drop that descriptions about sub nodes.
>[Kundanala, Praveen Teja] Okay.
>
> Thanks,
> Michal