[PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Alice Guo]

Add DT Binding doc for the Unique ID of i.MX 8M series.

Signed-off-by: Alice Guo <alice.guo@nxp.com>
---
 .../devicetree/bindings/arm/fsl.yaml          | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index e4db0f9ed664..f4faebbb57da 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -901,6 +901,31 @@ properties:
               - fsl,s32v234-evb           # S32V234-EVB2 Customer Evaluation Board
           - const: fsl,s32v234

+  soc:
+    type: object
+    properties:
+      compatible:
+        oneOf:
+          - description: i.MX8M SoCs
+            items:
+              - enum:
+                  - fsl,imx8mm-soc
+                  - fsl,imx8mn-soc
+                  - fsl,imx8mp-soc
+                  - fsl,imx8mq-soc
+              - const: simple-bus
+
+          - description: Other SoCs
+            items:
+              - const: simple-bus
+
+      nvmem-cells:
+        maxItems: 1
+        description: Phandle to the SOC Unique ID provided by a nvmem node
+
+      nvmem-cells-names:
+        const: soc_unique_id
+
 additionalProperties: true

 ...
--
2.17.1

[PATCH v3 2/4] arm64: dts: imx8m: add compatible string to .dtsi file

[Alice Guo]

Add compatible string to .dtsi file for binding of imx8_soc_init_driver
and device.

Signed-off-by: Alice Guo <alice.guo@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 | 2 +-
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 033fee525982..cd9a77d763eb 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -257,7 +257,7 @@
 	};

 	soc@0 {
-		compatible = "simple-bus";
+		compatible = "fsl,imx8mm-soc", "simple-bus";
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index a06d2a6268e6..6d3a809a00fd 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -241,7 +241,7 @@
 	};

 	soc@0 {
-		compatible = "simple-bus";
+		compatible = "fsl,imx8mn-soc", "simple-bus";
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 479312293036..9cd218f5f004 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -218,7 +218,7 @@
 	};

 	soc@0 {
-		compatible = "simple-bus";
+		compatible = "fsl,imx8mp-soc", "simple-bus";
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index a841a023e8e0..9b6d9307e5d7 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -286,7 +286,7 @@
 	};

 	soc@0 {
-		compatible = "simple-bus";
+		compatible = "fsl,imx8mq-soc", "simple-bus";
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
--
2.17.1

[PATCH v3 3/4] arm64: dts: imx8m: add nvmem-cell related stuff

[Alice Guo]

Add nvmem-cell related stuff for the soc unique ID.

Signed-off-by: Alice Guo <alice.guo@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 6 ++++++
 arch/arm64/boot/dts/freescale/imx8mn.dtsi | 6 ++++++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 6 ++++++
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 6 ++++++
 4 files changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index cd9a77d763eb..d4aec3eae147 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -261,6 +261,8 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
+		nvmem-cells = <&imx8mm_uid>;
+		nvmem-cell-names = "soc_unique_id";

 		aips1: bus@30000000 {
 			compatible = "fsl,aips-bus", "simple-bus";
@@ -518,6 +520,10 @@
 				#address-cells = <1>;
 				#size-cells = <1>;

+				imx8mm_uid: unique_id@410 {
+					reg = <0x4 0x8>;
+				};
+
 				cpu_speed_grade: speed-grade@10 {
 					reg = <0x10 4>;
 				};
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index 6d3a809a00fd..ff12194b60a1 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -245,6 +245,8 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
+		nvmem-cells = <&imx8mn_uid>;
+		nvmem-cell-names = "soc_unique_id";

 		aips1: bus@30000000 {
 			compatible = "fsl,aips-bus", "simple-bus";
@@ -388,6 +390,10 @@
 				#address-cells = <1>;
 				#size-cells = <1>;

+				imx8mn_uid: unique_id@410 {
+					reg = <0x4 0x8>;
+				};
+
 				cpu_speed_grade: speed-grade@10 {
 					reg = <0x10 4>;
 				};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9cd218f5f004..3db59f61e871 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -222,6 +222,8 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
+		nvmem-cells = <&imx8mp_uid>;
+		nvmem-cell-names = "soc_unique_id";

 		aips1: bus@30000000 {
 			compatible = "fsl,aips-bus", "simple-bus";
@@ -328,6 +330,10 @@
 				#address-cells = <1>;
 				#size-cells = <1>;

+				imx8mp_uid: unique_id@420 {
+					reg = <0x8 0x8>;
+				};
+
 				cpu_speed_grade: speed-grade@10 {
 					reg = <0x10 4>;
 				};
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 9b6d9307e5d7..6db5cba9c07d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -291,6 +291,8 @@
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x3e000000>;
 		dma-ranges = <0x40000000 0x0 0x40000000 0xc0000000>;
+		nvmem-cells = <&imx8mq_uid>;
+		nvmem-cell-names = "soc_unique_id";

 		bus@30000000 { /* AIPS1 */
 			compatible = "fsl,aips-bus", "simple-bus";
@@ -555,6 +557,10 @@
 				#address-cells = <1>;
 				#size-cells = <1>;

+				imx8mq_uid: soc_uid@410 {
+					reg = <0x4 0x8>;
+				};
+
 				cpu_speed_grade: speed-grade@10 {
 					reg = <0x10 4>;
 				};
--
2.17.1

[PATCH v3 4/4] soc: imx8m: change to use platform driver

[Alice Guo]

Directly reading ocotp register depends on that bootloader enables ocotp
clk, which is not always effective, so change to use nvmem API. Using
nvmem API requires to support driver defer probe and thus change
soc-imx8m.c to use platform driver.

The other reason is that directly reading ocotp register causes kexec
kernel hang because the 1st kernel running will disable unused clks
after kernel boots up, and then ocotp clk will be disabled even if
bootloader enables it. When kexec kernel, ocotp clk needs to be enabled
before reading ocotp registers, and nvmem API with platform driver
supported can accomplish this.

Old .dts files can also work.

Signed-off-by: Alice Guo <alice.guo@nxp.com>
---
 drivers/soc/imx/soc-imx8m.c | 89 ++++++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
index cc57a384d74d..af2c0dbe8291 100644
--- a/drivers/soc/imx/soc-imx8m.c
+++ b/drivers/soc/imx/soc-imx8m.c
@@ -5,6 +5,8 @@

 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of_address.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
@@ -29,7 +31,7 @@

 struct imx8_soc_data {
 	char *name;
-	u32 (*soc_revision)(void);
+	u32 (*soc_revision)(struct device *dev, int flag);
 };

 static u64 soc_uid;
@@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
 static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
 #endif

-static u32 __init imx8mq_soc_revision(void)
+static u32 __init imx8mq_soc_revision(struct device *dev, int flag)
 {
 	struct device_node *np;
 	void __iomem *ocotp_base;
@@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
 			rev = REV_B1;
 	}

-	soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
-	soc_uid <<= 32;
-	soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
+	if (flag) {
+		int ret = 0;
+
+		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
+		if (ret)
+			return ret;
+	} else {
+		soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
+		soc_uid <<= 32;
+		soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
+	}

 	iounmap(ocotp_base);
 	of_node_put(np);
@@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
 	of_node_put(np);
 }

-static u32 __init imx8mm_soc_revision(void)
+static u32 __init imx8mm_soc_revision(struct device *dev, int flag)
 {
 	struct device_node *np;
 	void __iomem *anatop_base;
@@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
 	iounmap(anatop_base);
 	of_node_put(np);

-	imx8mm_soc_uid();
+	if (flag) {
+		int ret = 0;
+
+		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
+		if (ret)
+			return ret;
+	} else {
+		imx8mm_soc_uid();
+	}

 	return rev;
 }
@@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = {
 	{ }
 };

+static __maybe_unused const struct of_device_id imx8m_soc_match[] = {
+	{ .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
+	{ .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
+	{ .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
+	{ .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, imx8m_soc_match);
+
 #define imx8_revision(soc_rev) \
 	soc_rev ? \
 	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev & 0xf) : \
 	"unknown"

-static int __init imx8_soc_init(void)
+static int imx8_soc_init_flag(struct platform_device *pdev, int flag)
 {
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
@@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
 	if (ret)
 		goto free_soc;

-	id = of_match_node(imx8_soc_match, of_root);
+	if (flag)
+		id = of_match_node(imx8m_soc_match, pdev->dev.of_node);
+	else
+		id = of_match_node(imx8_soc_match, of_root);
 	if (!id) {
 		ret = -ENODEV;
 		goto free_soc;
@@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
 	if (data) {
 		soc_dev_attr->soc_id = data->name;
 		if (data->soc_revision)
-			soc_rev = data->soc_revision();
+			soc_rev = data->soc_revision(&pdev->dev, flag);
+
+		if (flag) {
+			ret = soc_rev;
+			if (ret < 0)
+				goto free_soc;
+		}
 	}

 	soc_dev_attr->revision = imx8_revision(soc_rev);
@@ -230,4 +266,37 @@ static int __init imx8_soc_init(void)
 	kfree(soc_dev_attr);
 	return ret;
 }
+
+static int __init imx8_soc_init(void)
+{
+	int ret = 0, flag = 0;
+
+	if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
+	    of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
+	    of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
+	    of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))
+		return 0;
+
+	ret = imx8_soc_init_flag(NULL, flag);
+	return ret;
+}
 device_initcall(imx8_soc_init);
+
+static int imx8_soc_init_probe(struct platform_device *pdev)
+{
+	int ret = 0, flag = 1;
+
+	ret = imx8_soc_init_flag(pdev, flag);
+	return ret;
+}
+
+static struct platform_driver imx8_soc_init_driver = {
+	.probe = imx8_soc_init_probe,
+	.driver = {
+		.name = "imx8_soc_init",
+		.of_match_table = of_match_ptr(imx8m_soc_match),
+	},
+};
+
+module_platform_driver(imx8_soc_init_driver);
+MODULE_LICENSE("GPL v2");
--
2.17.1

Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Krzysztof Kozlowski]

On Fri, Nov 13, 2020 at 07:04:06PM +0800, Alice Guo wrote:
> Add DT Binding doc for the Unique ID of i.MX 8M series.
> 

Please align the subject prefix with history (git log --oneline).

What are the changes comparing to v2 and v1? Where is the changelog?

Best regards,
Krzysztof

Re: [PATCH v3 2/4] arm64: dts: imx8m: add compatible string to .dtsi file

[Krzysztof Kozlowski]

On Fri, Nov 13, 2020 at 07:04:07PM +0800, Alice Guo wrote:

Where is the changelog against v2?

Change the subject to: "arm64: dts: imx8m: add SoC ID compatible". It is
kind of obvious that you add it to DTSI files, so no need to put it into
the subject.

> Add compatible string to .dtsi file for binding of imx8_soc_init_driver
> and device.

s/file/files/

With above changes:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v3 3/4] arm64: dts: imx8m: add nvmem-cell related stuff

[Krzysztof Kozlowski]

On Fri, Nov 13, 2020 at 07:04:08PM +0800, Alice Guo wrote:
> Add nvmem-cell related stuff for the soc unique ID.

Do not silently ignore comments during review. Let me repeat my comment
from v1:

Subject and commit msg: please do not add "stuff" but describe what
are you adding and why (what is the purpose, feature, benefit).
"Stuff" is too generic.

I can rephrase it, since maybe I was not understood. Do not use words
like "stuff" or "thing". Instead describe exactly what you are adding
and answer to the most important question: why are you adding it.

Best regards,
Krzysztof

> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 6 ++++++
>  arch/arm64/boot/dts/freescale/imx8mn.dtsi | 6 ++++++
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 6 ++++++
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 6 ++++++
>  4 files changed, 24 insertions(+)

Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Krzysztof Kozlowski]

On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote:
> Directly reading ocotp register depends on that bootloader enables ocotp
> clk, which is not always effective, so change to use nvmem API. Using
> nvmem API requires to support driver defer probe and thus change
> soc-imx8m.c to use platform driver.
> 
> The other reason is that directly reading ocotp register causes kexec
> kernel hang because the 1st kernel running will disable unused clks
> after kernel boots up, and then ocotp clk will be disabled even if
> bootloader enables it. When kexec kernel, ocotp clk needs to be enabled
> before reading ocotp registers, and nvmem API with platform driver
> supported can accomplish this.
> 
> Old .dts files can also work.
> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
>  drivers/soc/imx/soc-imx8m.c | 89 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> index cc57a384d74d..af2c0dbe8291 100644
> --- a/drivers/soc/imx/soc-imx8m.c
> +++ b/drivers/soc/imx/soc-imx8m.c
> @@ -5,6 +5,8 @@
> 
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of_address.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
> @@ -29,7 +31,7 @@
> 
>  struct imx8_soc_data {
>  	char *name;
> -	u32 (*soc_revision)(void);
> +	u32 (*soc_revision)(struct device *dev, int flag);
>  };
> 
>  static u64 soc_uid;
> @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
>  static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
>  #endif
> 
> -static u32 __init imx8mq_soc_revision(void)
> +static u32 __init imx8mq_soc_revision(struct device *dev, int flag)
>  {
>  	struct device_node *np;
>  	void __iomem *ocotp_base;
> @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
>  			rev = REV_B1;
>  	}
> 
> -	soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> -	soc_uid <<= 32;
> -	soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> +	if (flag) {
> +		int ret = 0;
> +
> +		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> +		if (ret)
> +			return ret;
> +	} else {
> +		soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> +		soc_uid <<= 32;
> +		soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> +	}
> 
>  	iounmap(ocotp_base);
>  	of_node_put(np);
> @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
>  	of_node_put(np);
>  }
> 
> -static u32 __init imx8mm_soc_revision(void)
> +static u32 __init imx8mm_soc_revision(struct device *dev, int flag)
>  {
>  	struct device_node *np;
>  	void __iomem *anatop_base;
> @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
>  	iounmap(anatop_base);
>  	of_node_put(np);
> 
> -	imx8mm_soc_uid();
> +	if (flag) {
> +		int ret = 0;
> +
> +		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> +		if (ret)
> +			return ret;
> +	} else {
> +		imx8mm_soc_uid();
> +	}
> 
>  	return rev;
>  }
> @@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = {
>  	{ }
>  };
> 
> +static __maybe_unused const struct of_device_id imx8m_soc_match[] = {

Could this really be unused?

> +	{ .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
> +	{ .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
> +	{ .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
> +	{ .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, imx8m_soc_match);

You already have "imx8_soc_match" which covers imx8m and now you add
"imx8m_soc_match" which also covers imx8m. Such naming is a pure
confusion.

> +
>  #define imx8_revision(soc_rev) \
>  	soc_rev ? \
>  	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev & 0xf) : \
>  	"unknown"
> 
> -static int __init imx8_soc_init(void)
> +static int imx8_soc_init_flag(struct platform_device *pdev, int flag)
>  {
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
> @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
>  	if (ret)
>  		goto free_soc;
> 
> -	id = of_match_node(imx8_soc_match, of_root);
> +	if (flag)
> +		id = of_match_node(imx8m_soc_match, pdev->dev.of_node);
> +	else
> +		id = of_match_node(imx8_soc_match, of_root);
>  	if (!id) {
>  		ret = -ENODEV;
>  		goto free_soc;
> @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
>  	if (data) {
>  		soc_dev_attr->soc_id = data->name;
>  		if (data->soc_revision)
> -			soc_rev = data->soc_revision();
> +			soc_rev = data->soc_revision(&pdev->dev, flag);
> +
> +		if (flag) {
> +			ret = soc_rev;
> +			if (ret < 0)
> +				goto free_soc;
> +		}
>  	}
> 
>  	soc_dev_attr->revision = imx8_revision(soc_rev);
> @@ -230,4 +266,37 @@ static int __init imx8_soc_init(void)
>  	kfree(soc_dev_attr);
>  	return ret;
>  }
> +
> +static int __init imx8_soc_init(void)
> +{
> +	int ret = 0, flag = 0;
> +
> +	if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
> +	    of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
> +	    of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
> +	    of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))

Missing puts.

Don't duplicate the compatibles, iterate over existing structure... or
see comments below.  Maybe you could simplify it with something like
of_find_matching_node_and_match()... but check comments below.

> +		return 0;
> +
> +	ret = imx8_soc_init_flag(NULL, flag);
> +	return ret;
> +}
>  device_initcall(imx8_soc_init);

Where is the changelog? This was removed previously, now it stays...

After more thoughs, it looks you have kept it for the purpose of
supporting existing DTB, but it is not explained. Neither in the source
code (which after applying this patch looks confusing) nor in commit
message.

In case of old DTB without fsl,imx8mm-soc-like compatibles, it would be
better to still register a platform driver and create a device
(of_platform_device_create())). However still this won't solve the
problem of actually missing device node... so maybe this double entry
point is acceptable, if properly explained.

> +
> +static int imx8_soc_init_probe(struct platform_device *pdev)
> +{
> +	int ret = 0, flag = 1;
> +
> +	ret = imx8_soc_init_flag(pdev, flag);

Never name unspecified booleans like "flag". The same as string
variables should be named "string", integers should not be named
"number".

> +	return ret;
> +}
> +
> +static struct platform_driver imx8_soc_init_driver = {
> +	.probe = imx8_soc_init_probe,
> +	.driver = {
> +		.name = "imx8_soc_init",
> +		.of_match_table = of_match_ptr(imx8m_soc_match),

Can it bind without OF? Why it's a of_match_ptr()?

> +	},
> +};
> +
> +module_platform_driver(imx8_soc_init_driver);

Here and in all other places (including driver name) this is not a SoC
initialization (init) driver. You cannot initialize a SoC. This looks
like a SoC ID driver, so one name could be "imx8_soc_id".

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Krzysztof Kozlowski]

On Fri, Nov 13, 2020 at 07:04:06PM +0800, Alice Guo wrote:
> Add DT Binding doc for the Unique ID of i.MX 8M series.
> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
>  .../devicetree/bindings/arm/fsl.yaml          | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> index e4db0f9ed664..f4faebbb57da 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> @@ -901,6 +901,31 @@ properties:
>                - fsl,s32v234-evb           # S32V234-EVB2 Customer Evaluation Board
>            - const: fsl,s32v234
> 
> +  soc:
> +    type: object
> +    properties:
> +      compatible:
> +        oneOf:
> +          - description: i.MX8M SoCs
> +            items:
> +              - enum:
> +                  - fsl,imx8mm-soc
> +                  - fsl,imx8mn-soc
> +                  - fsl,imx8mp-soc
> +                  - fsl,imx8mq-soc
> +              - const: simple-bus
> +
> +          - description: Other SoCs
> +            items:
> +              - const: simple-bus
> +
> +      nvmem-cells:
> +        maxItems: 1
> +        description: Phandle to the SOC Unique ID provided by a nvmem node
> +
> +      nvmem-cells-names:
> +        const: soc_unique_id
> +
>  additionalProperties: true

The "soc" node should be required for these compatibles. Otherwise you
will have to stay with this
backwards-compatible-DTB-device-initcall-glue for many years... I think
you can achieve it with allOf.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月15日 0:50
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc
> for soc unique ID
> 
> Caution: EXT Email
> 
> On Fri, Nov 13, 2020 at 07:04:06PM +0800, Alice Guo wrote:
> > Add DT Binding doc for the Unique ID of i.MX 8M series.
> >
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > ---
> >  .../devicetree/bindings/arm/fsl.yaml          | 25
> +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml
> > b/Documentation/devicetree/bindings/arm/fsl.yaml
> > index e4db0f9ed664..f4faebbb57da 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > @@ -901,6 +901,31 @@ properties:
> >                - fsl,s32v234-evb           # S32V234-EVB2 Customer
> Evaluation Board
> >            - const: fsl,s32v234
> >
> > +  soc:
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        oneOf:
> > +          - description: i.MX8M SoCs
> > +            items:
> > +              - enum:
> > +                  - fsl,imx8mm-soc
> > +                  - fsl,imx8mn-soc
> > +                  - fsl,imx8mp-soc
> > +                  - fsl,imx8mq-soc
> > +              - const: simple-bus
> > +
> > +          - description: Other SoCs
> > +            items:
> > +              - const: simple-bus
> > +
> > +      nvmem-cells:
> > +        maxItems: 1
> > +        description: Phandle to the SOC Unique ID provided by a nvmem
> > + node
> > +
> > +      nvmem-cells-names:
> > +        const: soc_unique_id
> > +
> >  additionalProperties: true
> 
> The "soc" node should be required for these compatibles. Otherwise you will
> have to stay with this backwards-compatible-DTB-device-initcall-glue for many
> years... I think you can achieve it with allOf.
> 
[Alice Guo]Hi,
I find that my description for compatible is ambiguous. There are two kinds of compatible of the "soc" node:
1. For dtsi files used for SoCs other than imx8m series SoCs and old version dtsi files used for imx8m series SoCs,
compatible of the "soc" node is "simple-bus".
2. For new version dtsi files used for imx8m series SoCs, compatible of the "soc" node is {"fsl,imx8mX-soc","simple-bus"}.

"nvmem-cell" is an optional property.

I do not understand what you mean. You mean that limit the compatible of "soc" node must include " fsl,imx8mX-soc" in new version dts files. Is my understanding correct?

Best regards,
Alice

> Best regards,
> Krzysztof

RE: [EXT] Re: [PATCH v3 2/4] arm64: dts: imx8m: add compatible string to .dtsi file

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月14日 23:57
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH v3 2/4] arm64: dts: imx8m: add compatible string
> to .dtsi file
> 
> Caution: EXT Email
> 
> On Fri, Nov 13, 2020 at 07:04:07PM +0800, Alice Guo wrote:
> 
> Where is the changelog against v2?
> 
> Change the subject to: "arm64: dts: imx8m: add SoC ID compatible". It is kind
> of obvious that you add it to DTSI files, so no need to put it into the subject.
> 
> > Add compatible string to .dtsi file for binding of
> > imx8_soc_init_driver and device.
> 
> s/file/files/
> 
> With above changes:
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

[Alice Guo] Hi,
I will modify according to your comment.

Best regards,
Alice

> Best regards,
> Krzysztof

RE: [EXT] Re: [PATCH v3 3/4] arm64: dts: imx8m: add nvmem-cell related stuff

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月15日 0:01
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH v3 3/4] arm64: dts: imx8m: add nvmem-cell related
> stuff
> 
> Caution: EXT Email
> 
> On Fri, Nov 13, 2020 at 07:04:08PM +0800, Alice Guo wrote:
> > Add nvmem-cell related stuff for the soc unique ID.
> 
> Do not silently ignore comments during review. Let me repeat my comment
> from v1:
> 
> Subject and commit msg: please do not add "stuff" but describe what are you
> adding and why (what is the purpose, feature, benefit).
> "Stuff" is too generic.
> 
> I can rephrase it, since maybe I was not understood. Do not use words like
> "stuff" or "thing". Instead describe exactly what you are adding and answer to
> the most important question: why are you adding it.
> 
> Best regards,
> Krzysztof
> 
[Alice Guo] Hi,
I will modify according to your comment.
Best Regards,
Alice
> >
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 6 ++++++
> > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 6 ++++++
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 6 ++++++
> > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 6 ++++++
> >  4 files changed, 24 insertions(+)

RE: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月15日 0:41
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> 
> Caution: EXT Email
> 
> On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote:
> > Directly reading ocotp register depends on that bootloader enables
> > ocotp clk, which is not always effective, so change to use nvmem API.
> > Using nvmem API requires to support driver defer probe and thus change
> > soc-imx8m.c to use platform driver.
> >
> > The other reason is that directly reading ocotp register causes kexec
> > kernel hang because the 1st kernel running will disable unused clks
> > after kernel boots up, and then ocotp clk will be disabled even if
> > bootloader enables it. When kexec kernel, ocotp clk needs to be
> > enabled before reading ocotp registers, and nvmem API with platform
> > driver supported can accomplish this.
> >
> > Old .dts files can also work.
> >
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > ---
> >  drivers/soc/imx/soc-imx8m.c | 89
> > ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 79 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> > index cc57a384d74d..af2c0dbe8291 100644
> > --- a/drivers/soc/imx/soc-imx8m.c
> > +++ b/drivers/soc/imx/soc-imx8m.c
> > @@ -5,6 +5,8 @@
> >
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> >  #include <linux/of_address.h>
> >  #include <linux/slab.h>
> >  #include <linux/sys_soc.h>
> > @@ -29,7 +31,7 @@
> >
> >  struct imx8_soc_data {
> >       char *name;
> > -     u32 (*soc_revision)(void);
> > +     u32 (*soc_revision)(struct device *dev, int flag);
> >  };
> >
> >  static u64 soc_uid;
> > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> >  static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
> > #endif
> >
> > -static u32 __init imx8mq_soc_revision(void)
> > +static u32 __init imx8mq_soc_revision(struct device *dev, int flag)
> >  {
> >       struct device_node *np;
> >       void __iomem *ocotp_base;
> > @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
> >                       rev = REV_B1;
> >       }
> >
> > -     soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > -     soc_uid <<= 32;
> > -     soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > +     if (flag) {
> > +             int ret = 0;
> > +
> > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> &soc_uid);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > +             soc_uid <<= 32;
> > +             soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > +     }
> >
> >       iounmap(ocotp_base);
> >       of_node_put(np);
> > @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
> >       of_node_put(np);
> >  }
> >
> > -static u32 __init imx8mm_soc_revision(void)
> > +static u32 __init imx8mm_soc_revision(struct device *dev, int flag)
> >  {
> >       struct device_node *np;
> >       void __iomem *anatop_base;
> > @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
> >       iounmap(anatop_base);
> >       of_node_put(np);
> >
> > -     imx8mm_soc_uid();
> > +     if (flag) {
> > +             int ret = 0;
> > +
> > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> &soc_uid);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             imx8mm_soc_uid();
> > +     }
> >
> >       return rev;
> >  }
> > @@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id
> imx8_soc_match[] = {
> >       { }
> >  };
> >
> > +static __maybe_unused const struct of_device_id imx8m_soc_match[] = {
> 
> Could this really be unused?

[Alice Guo] I will delete "__maybe_unused".

> 
> > +     { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
> > +     { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
> > +     { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
> > +     { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> 
> You already have "imx8_soc_match" which covers imx8m and now you add
> "imx8m_soc_match" which also covers imx8m. Such naming is a pure
> confusion.
> 

[Alice Guo] device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. The purpose is to be compatible with the old DTS file which does not have
"fsl,imx8mX-soc".

> > +
> >  #define imx8_revision(soc_rev) \
> >       soc_rev ? \
> >       kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &
> 0xf) : \
> >       "unknown"
> >
> > -static int __init imx8_soc_init(void)
> > +static int imx8_soc_init_flag(struct platform_device *pdev, int flag)
> >  {
> >       struct soc_device_attribute *soc_dev_attr;
> >       struct soc_device *soc_dev;
> > @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
> >       if (ret)
> >               goto free_soc;
> >
> > -     id = of_match_node(imx8_soc_match, of_root);
> > +     if (flag)
> > +             id = of_match_node(imx8m_soc_match,
> pdev->dev.of_node);
> > +     else
> > +             id = of_match_node(imx8_soc_match, of_root);
> >       if (!id) {
> >               ret = -ENODEV;
> >               goto free_soc;
> > @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
> >       if (data) {
> >               soc_dev_attr->soc_id = data->name;
> >               if (data->soc_revision)
> > -                     soc_rev = data->soc_revision();
> > +                     soc_rev = data->soc_revision(&pdev->dev, flag);
> > +
> > +             if (flag) {
> > +                     ret = soc_rev;
> > +                     if (ret < 0)
> > +                             goto free_soc;
> > +             }
> >       }
> >
> >       soc_dev_attr->revision = imx8_revision(soc_rev); @@ -230,4
> > +266,37 @@ static int __init imx8_soc_init(void)
> >       kfree(soc_dev_attr);
> >       return ret;
> >  }
> > +
> > +static int __init imx8_soc_init(void) {
> > +     int ret = 0, flag = 0;
> > +
> > +     if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
> > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
> > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
> > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))
> 
> Missing puts.
> 
> Don't duplicate the compatibles, iterate over existing structure... or see
> comments below.  Maybe you could simplify it with something like
> of_find_matching_node_and_match()... but check comments below.

[Alice Guo] I check comments below.

> 
> > +             return 0;
> > +
> > +     ret = imx8_soc_init_flag(NULL, flag);
> > +     return ret;
> > +}
> >  device_initcall(imx8_soc_init);
> 
> Where is the changelog? This was removed previously, now it stays...
> 
> After more thoughs, it looks you have kept it for the purpose of supporting
> existing DTB, but it is not explained. Neither in the source code (which after
> applying this patch looks confusing) nor in commit message.
> 
> In case of old DTB without fsl,imx8mm-soc-like compatibles, it would be better
> to still register a platform driver and create a device
> (of_platform_device_create())). However still this won't solve the problem of
> actually missing device node... so maybe this double entry point is acceptable,
> if properly explained.

[Alice Guo] Sorry, I will add changelog next time. Actually I wrote "Old .dts files can also work." in the commit.

device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. Can I keep double entry point?

> 
> > +
> > +static int imx8_soc_init_probe(struct platform_device *pdev) {
> > +     int ret = 0, flag = 1;
> > +
> > +     ret = imx8_soc_init_flag(pdev, flag);
> 
> Never name unspecified booleans like "flag". The same as string variables
> should be named "string", integers should not be named "number".

[Alice Guo] Ok. I will modify it. Can the name of function use suffix "_flag"?

> 
> > +     return ret;
> > +}
> > +
> > +static struct platform_driver imx8_soc_init_driver = {
> > +     .probe = imx8_soc_init_probe,
> > +     .driver = {
> > +             .name = "imx8_soc_init",
> > +             .of_match_table = of_match_ptr(imx8m_soc_match),
> 
> Can it bind without OF? Why it's a of_match_ptr()?
> 

[Alice Guo] I will modify it.

> > +     },
> > +};
> > +
> > +module_platform_driver(imx8_soc_init_driver);
> 
> Here and in all other places (including driver name) this is not a SoC
> initialization (init) driver. You cannot initialize a SoC. This looks like a SoC ID
> driver, so one name could be "imx8_soc_id".
> 

[Alice Guo] I will modify it.

Best regards,
Alice Guo

> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Krzysztof Kozlowski]

On Mon, Nov 16, 2020 at 07:04:13AM +0000, Alice Guo wrote:
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk@kernel.org>
> > Sent: 2020年11月15日 0:50
> > To: Alice Guo <alice.guo@nxp.com>
> > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc
> > for soc unique ID
> > 
> > Caution: EXT Email
> > 
> > On Fri, Nov 13, 2020 at 07:04:06PM +0800, Alice Guo wrote:
> > > Add DT Binding doc for the Unique ID of i.MX 8M series.
> > >
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > ---
> > >  .../devicetree/bindings/arm/fsl.yaml          | 25
> > +++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml
> > > b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > index e4db0f9ed664..f4faebbb57da 100644
> > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > @@ -901,6 +901,31 @@ properties:
> > >                - fsl,s32v234-evb           # S32V234-EVB2 Customer
> > Evaluation Board
> > >            - const: fsl,s32v234
> > >
> > > +  soc:
> > > +    type: object
> > > +    properties:
> > > +      compatible:
> > > +        oneOf:
> > > +          - description: i.MX8M SoCs
> > > +            items:
> > > +              - enum:
> > > +                  - fsl,imx8mm-soc
> > > +                  - fsl,imx8mn-soc
> > > +                  - fsl,imx8mp-soc
> > > +                  - fsl,imx8mq-soc
> > > +              - const: simple-bus
> > > +
> > > +          - description: Other SoCs
> > > +            items:
> > > +              - const: simple-bus
> > > +
> > > +      nvmem-cells:
> > > +        maxItems: 1
> > > +        description: Phandle to the SOC Unique ID provided by a nvmem
> > > + node
> > > +
> > > +      nvmem-cells-names:
> > > +        const: soc_unique_id
> > > +
> > >  additionalProperties: true
> > 
> > The "soc" node should be required for these compatibles. Otherwise you will
> > have to stay with this backwards-compatible-DTB-device-initcall-glue for many
> > years... I think you can achieve it with allOf.
> > 
> [Alice Guo]Hi,
> I find that my description for compatible is ambiguous. There are two kinds of compatible of the "soc" node:
> 1. For dtsi files used for SoCs other than imx8m series SoCs and old version dtsi files used for imx8m series SoCs,
> compatible of the "soc" node is "simple-bus".
> 2. For new version dtsi files used for imx8m series SoCs, compatible of the "soc" node is {"fsl,imx8mX-soc","simple-bus"}.
> 
> "nvmem-cell" is an optional property.
> 
> I do not understand what you mean. You mean that limit the compatible of "soc" node must include " fsl,imx8mX-soc" in new version dts files. Is my understanding correct?

All new DTS files should have soc node with "fsl,imx8mX-soc" and this
should be required by dtschema. The nvmem-cells and related properties
do not look like optional. From the hardware perspective - they are
always present. From the driver point of view: they are required and
driver will fail to work.

Best regards,
Krzysztof

Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Krzysztof Kozlowski]

On Mon, Nov 16, 2020 at 08:18:59AM +0000, Alice Guo wrote:
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk@kernel.org>
> > Sent: 2020年11月15日 0:41
> > To: Alice Guo <alice.guo@nxp.com>
> > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> > 
> > Caution: EXT Email
> > 
> > On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote:
> > > Directly reading ocotp register depends on that bootloader enables
> > > ocotp clk, which is not always effective, so change to use nvmem API.
> > > Using nvmem API requires to support driver defer probe and thus change
> > > soc-imx8m.c to use platform driver.
> > >
> > > The other reason is that directly reading ocotp register causes kexec
> > > kernel hang because the 1st kernel running will disable unused clks
> > > after kernel boots up, and then ocotp clk will be disabled even if
> > > bootloader enables it. When kexec kernel, ocotp clk needs to be
> > > enabled before reading ocotp registers, and nvmem API with platform
> > > driver supported can accomplish this.
> > >
> > > Old .dts files can also work.
> > >
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > ---
> > >  drivers/soc/imx/soc-imx8m.c | 89
> > > ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 79 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> > > index cc57a384d74d..af2c0dbe8291 100644
> > > --- a/drivers/soc/imx/soc-imx8m.c
> > > +++ b/drivers/soc/imx/soc-imx8m.c
> > > @@ -5,6 +5,8 @@
> > >
> > >  #include <linux/init.h>
> > >  #include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/nvmem-consumer.h>
> > >  #include <linux/of_address.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/sys_soc.h>
> > > @@ -29,7 +31,7 @@
> > >
> > >  struct imx8_soc_data {
> > >       char *name;
> > > -     u32 (*soc_revision)(void);
> > > +     u32 (*soc_revision)(struct device *dev, int flag);
> > >  };
> > >
> > >  static u64 soc_uid;
> > > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> > >  static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
> > > #endif
> > >
> > > -static u32 __init imx8mq_soc_revision(void)
> > > +static u32 __init imx8mq_soc_revision(struct device *dev, int flag)
> > >  {
> > >       struct device_node *np;
> > >       void __iomem *ocotp_base;
> > > @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
> > >                       rev = REV_B1;
> > >       }
> > >
> > > -     soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > > -     soc_uid <<= 32;
> > > -     soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > > +     if (flag) {
> > > +             int ret = 0;
> > > +
> > > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > &soc_uid);
> > > +             if (ret)
> > > +                     return ret;
> > > +     } else {
> > > +             soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > > +             soc_uid <<= 32;
> > > +             soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > > +     }
> > >
> > >       iounmap(ocotp_base);
> > >       of_node_put(np);
> > > @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
> > >       of_node_put(np);
> > >  }
> > >
> > > -static u32 __init imx8mm_soc_revision(void)
> > > +static u32 __init imx8mm_soc_revision(struct device *dev, int flag)
> > >  {
> > >       struct device_node *np;
> > >       void __iomem *anatop_base;
> > > @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
> > >       iounmap(anatop_base);
> > >       of_node_put(np);
> > >
> > > -     imx8mm_soc_uid();
> > > +     if (flag) {
> > > +             int ret = 0;
> > > +
> > > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > &soc_uid);
> > > +             if (ret)
> > > +                     return ret;
> > > +     } else {
> > > +             imx8mm_soc_uid();
> > > +     }
> > >
> > >       return rev;
> > >  }
> > > @@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id
> > imx8_soc_match[] = {
> > >       { }
> > >  };
> > >
> > > +static __maybe_unused const struct of_device_id imx8m_soc_match[] = {
> > 
> > Could this really be unused?
> 
> [Alice Guo] I will delete "__maybe_unused".
> 
> > 
> > > +     { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
> > > +     { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
> > > +     { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
> > > +     { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> > 
> > You already have "imx8_soc_match" which covers imx8m and now you add
> > "imx8m_soc_match" which also covers imx8m. Such naming is a pure
> > confusion.
> > 
> 
> [Alice Guo] device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
> whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. The purpose is to be compatible with the old DTS file which does not have
> "fsl,imx8mX-soc".

I got it, but it's not what I was pointing out. Let me make it simpler:

  static const struct of_device_id imx8m_soc_match;
  static const struct of_device_id imx8_soc_match;

This is pure confusion in naming.

Based on this naming:
1. imx8m_soc_match means "matching only i.MX 8M SoCs",
2. imx8_soc_match means "match all of i.MX 8".

Totally different than what you wrote here and what you intend....

> 
> > > +
> > >  #define imx8_revision(soc_rev) \
> > >       soc_rev ? \
> > >       kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &
> > 0xf) : \
> > >       "unknown"
> > >
> > > -static int __init imx8_soc_init(void)
> > > +static int imx8_soc_init_flag(struct platform_device *pdev, int flag)
> > >  {
> > >       struct soc_device_attribute *soc_dev_attr;
> > >       struct soc_device *soc_dev;
> > > @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
> > >       if (ret)
> > >               goto free_soc;
> > >
> > > -     id = of_match_node(imx8_soc_match, of_root);
> > > +     if (flag)
> > > +             id = of_match_node(imx8m_soc_match,
> > pdev->dev.of_node);
> > > +     else
> > > +             id = of_match_node(imx8_soc_match, of_root);
> > >       if (!id) {
> > >               ret = -ENODEV;
> > >               goto free_soc;
> > > @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
> > >       if (data) {
> > >               soc_dev_attr->soc_id = data->name;
> > >               if (data->soc_revision)
> > > -                     soc_rev = data->soc_revision();
> > > +                     soc_rev = data->soc_revision(&pdev->dev, flag);
> > > +
> > > +             if (flag) {
> > > +                     ret = soc_rev;
> > > +                     if (ret < 0)
> > > +                             goto free_soc;
> > > +             }
> > >       }
> > >
> > >       soc_dev_attr->revision = imx8_revision(soc_rev); @@ -230,4
> > > +266,37 @@ static int __init imx8_soc_init(void)
> > >       kfree(soc_dev_attr);
> > >       return ret;
> > >  }
> > > +
> > > +static int __init imx8_soc_init(void) {
> > > +     int ret = 0, flag = 0;
> > > +
> > > +     if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
> > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
> > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
> > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))
> > 
> > Missing puts.
> > 
> > Don't duplicate the compatibles, iterate over existing structure... or see
> > comments below.  Maybe you could simplify it with something like
> > of_find_matching_node_and_match()... but check comments below.
> 
> [Alice Guo] I check comments below.
> 
> > 
> > > +             return 0;
> > > +
> > > +     ret = imx8_soc_init_flag(NULL, flag);
> > > +     return ret;
> > > +}
> > >  device_initcall(imx8_soc_init);
> > 
> > Where is the changelog? This was removed previously, now it stays...
> > 
> > After more thoughs, it looks you have kept it for the purpose of supporting
> > existing DTB, but it is not explained. Neither in the source code (which after
> > applying this patch looks confusing) nor in commit message.
> > 
> > In case of old DTB without fsl,imx8mm-soc-like compatibles, it would be better
> > to still register a platform driver and create a device
> > (of_platform_device_create())). However still this won't solve the problem of
> > actually missing device node... so maybe this double entry point is acceptable,
> > if properly explained.
> 
> [Alice Guo] Sorry, I will add changelog next time. Actually I wrote "Old .dts files can also work." in the commit.
> 
> device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
> whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. Can I keep double entry point?

If it is properly explained and there is no other way then yes, you
could. Here, for old DTBs, I would prefer to use
of_platform_device_create() and bind to "soc" node (child of root).
This way you would always have device and exactly one entry point for
the probe.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月17日 0:03
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT Binding
> doc for soc unique ID
> 
> Caution: EXT Email
> 
> On Mon, Nov 16, 2020 at 07:04:13AM +0000, Alice Guo wrote:
> >
> > > -----Original Message-----
> > > From: Krzysztof Kozlowski <krzk@kernel.org>
> > > Sent: 2020年11月15日 0:50
> > > To: Alice Guo <alice.guo@nxp.com>
> > > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org
> > > Subject: [EXT] Re: [PATCH v3 1/4] dt-bindings: soc: imx8m: add DT
> > > Binding doc for soc unique ID
> > >
> > > Caution: EXT Email
> > >
> > > On Fri, Nov 13, 2020 at 07:04:06PM +0800, Alice Guo wrote:
> > > > Add DT Binding doc for the Unique ID of i.MX 8M series.
> > > >
> > > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/arm/fsl.yaml          | 25
> > > +++++++++++++++++++
> > > >  1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > index e4db0f9ed664..f4faebbb57da 100644
> > > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > @@ -901,6 +901,31 @@ properties:
> > > >                - fsl,s32v234-evb           # S32V234-EVB2
> Customer
> > > Evaluation Board
> > > >            - const: fsl,s32v234
> > > >
> > > > +  soc:
> > > > +    type: object
> > > > +    properties:
> > > > +      compatible:
> > > > +        oneOf:
> > > > +          - description: i.MX8M SoCs
> > > > +            items:
> > > > +              - enum:
> > > > +                  - fsl,imx8mm-soc
> > > > +                  - fsl,imx8mn-soc
> > > > +                  - fsl,imx8mp-soc
> > > > +                  - fsl,imx8mq-soc
> > > > +              - const: simple-bus
> > > > +
> > > > +          - description: Other SoCs
> > > > +            items:
> > > > +              - const: simple-bus
> > > > +
> > > > +      nvmem-cells:
> > > > +        maxItems: 1
> > > > +        description: Phandle to the SOC Unique ID provided by a
> > > > + nvmem node
> > > > +
> > > > +      nvmem-cells-names:
> > > > +        const: soc_unique_id
> > > > +
> > > >  additionalProperties: true
> > >
> > > The "soc" node should be required for these compatibles. Otherwise
> > > you will have to stay with this
> > > backwards-compatible-DTB-device-initcall-glue for many years... I think you
> can achieve it with allOf.
> > >
> > [Alice Guo]Hi,
> > I find that my description for compatible is ambiguous. There are two kinds of
> compatible of the "soc" node:
> > 1. For dtsi files used for SoCs other than imx8m series SoCs and old
> > version dtsi files used for imx8m series SoCs, compatible of the "soc" node is
> "simple-bus".
> > 2. For new version dtsi files used for imx8m series SoCs, compatible of the
> "soc" node is {"fsl,imx8mX-soc","simple-bus"}.
> >
> > "nvmem-cell" is an optional property.
> >
> > I do not understand what you mean. You mean that limit the compatible of
> "soc" node must include " fsl,imx8mX-soc" in new version dts files. Is my
> understanding correct?
> 
> All new DTS files should have soc node with "fsl,imx8mX-soc" and this should be
> required by dtschema. The nvmem-cells and related properties do not look like
> optional. From the hardware perspective - they are always present. From the
> driver point of view: they are required and driver will fail to work.

[Alice Guo] 
[Alice Guo] 
+  soc:
+    type: object
+    properties:
+      compatible:
+        oneOf:
+          - description: new version DTS for i.MX8M SoCs
+            items:
+              - enum:
+                  - fsl,imx8mm-soc
+                  - fsl,imx8mn-soc
+                  - fsl,imx8mp-soc
+                  - fsl,imx8mq-soc
+              - const: simple-bus
+
+          - description: other SoCs and old version DTS for i.MX8M SoCs
+            items:
+              - const: simple-bus
+
+      nvmem-cells:
+        maxItems: 1
+        description: Phandle to the SOC Unique ID provided by a nvmem 
+ node
+
+      nvmem-cells-names:
+        const: soc_unique_id
+
+    allOf:
+      - if:
+          properties:
+            compatible:
+              contains:
+                enum:
+                  - fsl,imx8mm-soc
+                  - fsl,imx8mn-soc
+                  - fsl,imx8mp-soc
+                  - fsl,imx8mq-soc
+                const: simple-bus
+
+        then:
+          required:
+            - nvmem-cells
+            - nvmem-cells-names

The above is my modification. Is that ok?

Best regards,
Alice
> Best regards,
> Krzysztof

RE: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月17日 0:14
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> 
> Caution: EXT Email
> 
> On Mon, Nov 16, 2020 at 08:18:59AM +0000, Alice Guo wrote:
> >
> >
> > > -----Original Message-----
> > > From: Krzysztof Kozlowski <krzk@kernel.org>
> > > Sent: 2020年11月15日 0:41
> > > To: Alice Guo <alice.guo@nxp.com>
> > > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org
> > > Subject: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform
> > > driver
> > >
> > > Caution: EXT Email
> > >
> > > On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote:
> > > > Directly reading ocotp register depends on that bootloader enables
> > > > ocotp clk, which is not always effective, so change to use nvmem API.
> > > > Using nvmem API requires to support driver defer probe and thus
> > > > change soc-imx8m.c to use platform driver.
> > > >
> > > > The other reason is that directly reading ocotp register causes
> > > > kexec kernel hang because the 1st kernel running will disable
> > > > unused clks after kernel boots up, and then ocotp clk will be
> > > > disabled even if bootloader enables it. When kexec kernel, ocotp
> > > > clk needs to be enabled before reading ocotp registers, and nvmem
> > > > API with platform driver supported can accomplish this.
> > > >
> > > > Old .dts files can also work.
> > > >
> > > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > > ---
> > > >  drivers/soc/imx/soc-imx8m.c | 89
> > > > ++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 79 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/imx/soc-imx8m.c
> > > > b/drivers/soc/imx/soc-imx8m.c index cc57a384d74d..af2c0dbe8291
> > > > 100644
> > > > --- a/drivers/soc/imx/soc-imx8m.c
> > > > +++ b/drivers/soc/imx/soc-imx8m.c
> > > > @@ -5,6 +5,8 @@
> > > >
> > > >  #include <linux/init.h>
> > > >  #include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/nvmem-consumer.h>
> > > >  #include <linux/of_address.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/sys_soc.h>
> > > > @@ -29,7 +31,7 @@
> > > >
> > > >  struct imx8_soc_data {
> > > >       char *name;
> > > > -     u32 (*soc_revision)(void);
> > > > +     u32 (*soc_revision)(struct device *dev, int flag);
> > > >  };
> > > >
> > > >  static u64 soc_uid;
> > > > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> > > >  static inline u32 imx8mq_soc_revision_from_atf(void) { return 0;
> > > > }; #endif
> > > >
> > > > -static u32 __init imx8mq_soc_revision(void)
> > > > +static u32 __init imx8mq_soc_revision(struct device *dev, int
> > > > +flag)
> > > >  {
> > > >       struct device_node *np;
> > > >       void __iomem *ocotp_base;
> > > > @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
> > > >                       rev = REV_B1;
> > > >       }
> > > >
> > > > -     soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > > > -     soc_uid <<= 32;
> > > > -     soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > > > +     if (flag) {
> > > > +             int ret = 0;
> > > > +
> > > > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > > &soc_uid);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     } else {
> > > > +             soc_uid = readl_relaxed(ocotp_base +
> OCOTP_UID_HIGH);
> > > > +             soc_uid <<= 32;
> > > > +             soc_uid |= readl_relaxed(ocotp_base +
> OCOTP_UID_LOW);
> > > > +     }
> > > >
> > > >       iounmap(ocotp_base);
> > > >       of_node_put(np);
> > > > @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
> > > >       of_node_put(np);
> > > >  }
> > > >
> > > > -static u32 __init imx8mm_soc_revision(void)
> > > > +static u32 __init imx8mm_soc_revision(struct device *dev, int
> > > > +flag)
> > > >  {
> > > >       struct device_node *np;
> > > >       void __iomem *anatop_base;
> > > > @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
> > > >       iounmap(anatop_base);
> > > >       of_node_put(np);
> > > >
> > > > -     imx8mm_soc_uid();
> > > > +     if (flag) {
> > > > +             int ret = 0;
> > > > +
> > > > +             ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > > &soc_uid);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     } else {
> > > > +             imx8mm_soc_uid();
> > > > +     }
> > > >
> > > >       return rev;
> > > >  }
> > > > @@ -158,12 +176,21 @@ static __maybe_unused const struct
> > > > of_device_id
> > > imx8_soc_match[] = {
> > > >       { }
> > > >  };
> > > >
> > > > +static __maybe_unused const struct of_device_id imx8m_soc_match[]
> > > > += {
> > >
> > > Could this really be unused?
> >
> > [Alice Guo] I will delete "__maybe_unused".
> >
> > >
> > > > +     { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
> > > > +     { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
> > > > +     { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
> > > > +     { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
> > > > +     { }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> > >
> > > You already have "imx8_soc_match" which covers imx8m and now you add
> > > "imx8m_soc_match" which also covers imx8m. Such naming is a pure
> > > confusion.
> > >
> >
> > [Alice Guo] device_initcall is executed earlier than
> > module_platform_driver. imx8_soc_init will judge whether there is
> > "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit
> device_initcall and use module_platform_driver. The purpose is to be
> compatible with the old DTS file which does not have "fsl,imx8mX-soc".
> 
> I got it, but it's not what I was pointing out. Let me make it simpler:
> 
>   static const struct of_device_id imx8m_soc_match;
>   static const struct of_device_id imx8_soc_match;
> 
> This is pure confusion in naming.
> 
> Based on this naming:
> 1. imx8m_soc_match means "matching only i.MX 8M SoCs", 2. imx8_soc_match
> means "match all of i.MX 8".
> 
> Totally different than what you wrote here and what you intend....
> 
> >
> > > > +
> > > >  #define imx8_revision(soc_rev) \
> > > >       soc_rev ? \
> > > >       kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,
> > > > soc_rev &
> > > 0xf) : \
> > > >       "unknown"
> > > >
> > > > -static int __init imx8_soc_init(void)
> > > > +static int imx8_soc_init_flag(struct platform_device *pdev, int
> > > > +flag)
> > > >  {
> > > >       struct soc_device_attribute *soc_dev_attr;
> > > >       struct soc_device *soc_dev;
> > > > @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
> > > >       if (ret)
> > > >               goto free_soc;
> > > >
> > > > -     id = of_match_node(imx8_soc_match, of_root);
> > > > +     if (flag)
> > > > +             id = of_match_node(imx8m_soc_match,
> > > pdev->dev.of_node);
> > > > +     else
> > > > +             id = of_match_node(imx8_soc_match, of_root);
> > > >       if (!id) {
> > > >               ret = -ENODEV;
> > > >               goto free_soc;
> > > > @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
> > > >       if (data) {
> > > >               soc_dev_attr->soc_id = data->name;
> > > >               if (data->soc_revision)
> > > > -                     soc_rev = data->soc_revision();
> > > > +                     soc_rev = data->soc_revision(&pdev->dev,
> > > > + flag);
> > > > +
> > > > +             if (flag) {
> > > > +                     ret = soc_rev;
> > > > +                     if (ret < 0)
> > > > +                             goto free_soc;
> > > > +             }
> > > >       }
> > > >
> > > >       soc_dev_attr->revision = imx8_revision(soc_rev); @@ -230,4
> > > > +266,37 @@ static int __init imx8_soc_init(void)
> > > >       kfree(soc_dev_attr);
> > > >       return ret;
> > > >  }
> > > > +
> > > > +static int __init imx8_soc_init(void) {
> > > > +     int ret = 0, flag = 0;
> > > > +
> > > > +     if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
> > > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
> > > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
> > > > +         of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))
> > >
> > > Missing puts.
> > >
> > > Don't duplicate the compatibles, iterate over existing structure...
> > > or see comments below.  Maybe you could simplify it with something
> > > like of_find_matching_node_and_match()... but check comments below.
> >
> > [Alice Guo] I check comments below.
> >
> > >
> > > > +             return 0;
> > > > +
> > > > +     ret = imx8_soc_init_flag(NULL, flag);
> > > > +     return ret;
> > > > +}
> > > >  device_initcall(imx8_soc_init);
> > >
> > > Where is the changelog? This was removed previously, now it stays...
> > >
> > > After more thoughs, it looks you have kept it for the purpose of
> > > supporting existing DTB, but it is not explained. Neither in the
> > > source code (which after applying this patch looks confusing) nor in commit
> message.
> > >
> > > In case of old DTB without fsl,imx8mm-soc-like compatibles, it would
> > > be better to still register a platform driver and create a device
> > > (of_platform_device_create())). However still this won't solve the
> > > problem of actually missing device node... so maybe this double
> > > entry point is acceptable, if properly explained.
> >
> > [Alice Guo] Sorry, I will add changelog next time. Actually I wrote "Old .dts files
> can also work." in the commit.
> >
> > device_initcall is executed earlier than module_platform_driver.
> > imx8_soc_init will judge whether there is "fsl,imx8mX-soc" in DTS file. If there
> is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver.
> Can I keep double entry point?
> 
> If it is properly explained and there is no other way then yes, you could. Here, for
> old DTBs, I would prefer to use
> of_platform_device_create() and bind to "soc" node (child of root).
> This way you would always have device and exactly one entry point for the
> probe.
> 

static struct platform_driver imx8_soc_init_driver = {
	.probe = imx8_soc_init_probe,
	.driver = {
		.name = "soc@0",
	},
};
Can I use "soc@0" to match this driver? It will not use of_platform_device_create(). It will use of_find_property() to determine whether
and nvmem-cells can be used. If there is no nvmem-cells, it will use the old way, which supports old DTBS. There is no need to add new
compatible.

Best regards,
Alice

> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Krzysztof Kozlowski]

On Wed, Nov 18, 2020 at 10:28:47AM +0000, Alice Guo wrote:
 > 
> > If it is properly explained and there is no other way then yes, you could. Here, for
> > old DTBs, I would prefer to use
> > of_platform_device_create() and bind to "soc" node (child of root).
> > This way you would always have device and exactly one entry point for the
> > probe.
> > 
> 
> static struct platform_driver imx8_soc_init_driver = {
> 	.probe = imx8_soc_init_probe,
> 	.driver = {
> 		.name = "soc@0",
> 	},
> };
> Can I use "soc@0" to match this driver? It will not use of_platform_device_create(). It will use of_find_property() to determine whether
> and nvmem-cells can be used. If there is no nvmem-cells, it will use the old way, which supports old DTBS. There is no need to add new
> compatible.

No, the soc@0 is not a proper name for the driver.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月18日 18:42
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform
> driver
> 
> Caution: EXT Email
> 
> On Wed, Nov 18, 2020 at 10:28:47AM +0000, Alice Guo wrote:
>  >
> > > If it is properly explained and there is no other way then yes, you
> > > could. Here, for old DTBs, I would prefer to use
> > > of_platform_device_create() and bind to "soc" node (child of root).
> > > This way you would always have device and exactly one entry point
> > > for the probe.
> > >
> >
> > static struct platform_driver imx8_soc_init_driver = {
> >       .probe = imx8_soc_init_probe,
> >       .driver = {
> >               .name = "soc@0",
> >       },
> > };
> > Can I use "soc@0" to match this driver? It will not use
> > of_platform_device_create(). It will use of_find_property() to
> > determine whether and nvmem-cells can be used. If there is no nvmem-cells,
> it will use the old way, which supports old DTBS. There is no need to add new
> compatible.
> 
> No, the soc@0 is not a proper name for the driver.

I have no good idea, please give suggestion. Should I still add new compatible?
Should I still keep device_initcall? If use of_platform_device_create(), which
node should I use?

Best regards,
Alice Guo

> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Krzysztof Kozlowski]

On Wed, Nov 18, 2020 at 02:07:41PM +0000, Alice Guo wrote:
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk@kernel.org>
> > Sent: 2020年11月18日 18:42
> > To: Alice Guo <alice.guo@nxp.com>
> > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform
> > driver
> > 
> > Caution: EXT Email
> > 
> > On Wed, Nov 18, 2020 at 10:28:47AM +0000, Alice Guo wrote:
> >  >
> > > > If it is properly explained and there is no other way then yes, you
> > > > could. Here, for old DTBs, I would prefer to use
> > > > of_platform_device_create() and bind to "soc" node (child of root).
> > > > This way you would always have device and exactly one entry point
> > > > for the probe.
> > > >
> > >
> > > static struct platform_driver imx8_soc_init_driver = {
> > >       .probe = imx8_soc_init_probe,
> > >       .driver = {
> > >               .name = "soc@0",
> > >       },
> > > };
> > > Can I use "soc@0" to match this driver? It will not use
> > > of_platform_device_create(). It will use of_find_property() to
> > > determine whether and nvmem-cells can be used. If there is no nvmem-cells,
> > it will use the old way, which supports old DTBS. There is no need to add new
> > compatible.
> > 
> > No, the soc@0 is not a proper name for the driver.
> 
> I have no good idea, please give suggestion. Should I still add new compatible?
> Should I still keep device_initcall? If use of_platform_device_create(), which
> node should I use?

I mentioned my idea in the email before - of_platform_device_create() to
bind to the soc node. This will have to be in the initcall, you don't
have a choice to avoid it, since there was no compatible before.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Alice Guo]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月18日 22:11
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> 
> Caution: EXT Email
> 
> On Wed, Nov 18, 2020 at 02:07:41PM +0000, Alice Guo wrote:
> >
> >
> > > -----Original Message-----
> > > From: Krzysztof Kozlowski <krzk@kernel.org>
> > > Sent: 2020年11月18日 18:42
> > > To: Alice Guo <alice.guo@nxp.com>
> > > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use
> > > platform driver
> > >
> > > Caution: EXT Email
> > >
> > > On Wed, Nov 18, 2020 at 10:28:47AM +0000, Alice Guo wrote:
> > >  >
> > > > > If it is properly explained and there is no other way then yes,
> > > > > you could. Here, for old DTBs, I would prefer to use
> > > > > of_platform_device_create() and bind to "soc" node (child of root).
> > > > > This way you would always have device and exactly one entry
> > > > > point for the probe.
> > > > >
> > > >
> > > > static struct platform_driver imx8_soc_init_driver = {
> > > >       .probe = imx8_soc_init_probe,
> > > >       .driver = {
> > > >               .name = "soc@0",
> > > >       },
> > > > };
> > > > Can I use "soc@0" to match this driver? It will not use
> > > > of_platform_device_create(). It will use of_find_property() to
> > > > determine whether and nvmem-cells can be used. If there is no
> > > > nvmem-cells,
> > > it will use the old way, which supports old DTBS. There is no need
> > > to add new compatible.
> > >
> > > No, the soc@0 is not a proper name for the driver.
> >
> > I have no good idea, please give suggestion. Should I still add new compatible?
> > Should I still keep device_initcall? If use
> > of_platform_device_create(), which node should I use?
> 
> I mentioned my idea in the email before - of_platform_device_create() to bind
> to the soc node. This will have to be in the initcall, you don't have a choice to
> avoid it, since there was no compatible before.
>

	node = of_find_node_by_path("/soc@0");
	if (!node)
		return -ENODEV;

	pdev = of_platform_device_create(node, "XXX", NULL);
	if (!pdev)
		return -ENODEV;

Cannot use of_platform_device_create because "of_node_test_and_set_flag(np, OF_POPULATED)" returns true.
of_platform_device_create is used to create platform device, but soc@0 is created by common code. I don't know how
to bind to the soc node. The way I did in v3 seems not bad, it can work correctly and support old DTBs. Can I keep this way?

Best regards,
Alice Guo

> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver

[Krzysztof Kozlowski]

On Thu, Nov 19, 2020 at 07:32:17AM +0000, Alice Guo wrote:
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk@kernel.org>
> > Sent: 2020年11月18日 22:11
> > To: Alice Guo <alice.guo@nxp.com>
> > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> > 
> > Caution: EXT Email
> > 
> > On Wed, Nov 18, 2020 at 02:07:41PM +0000, Alice Guo wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Krzysztof Kozlowski <krzk@kernel.org>
> > > > Sent: 2020年11月18日 18:42
> > > > To: Alice Guo <alice.guo@nxp.com>
> > > > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > linux-arm-kernel@lists.infradead.org
> > > > Subject: Re: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use
> > > > platform driver
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Wed, Nov 18, 2020 at 10:28:47AM +0000, Alice Guo wrote:
> > > >  >
> > > > > > If it is properly explained and there is no other way then yes,
> > > > > > you could. Here, for old DTBs, I would prefer to use
> > > > > > of_platform_device_create() and bind to "soc" node (child of root).
> > > > > > This way you would always have device and exactly one entry
> > > > > > point for the probe.
> > > > > >
> > > > >
> > > > > static struct platform_driver imx8_soc_init_driver = {
> > > > >       .probe = imx8_soc_init_probe,
> > > > >       .driver = {
> > > > >               .name = "soc@0",
> > > > >       },
> > > > > };
> > > > > Can I use "soc@0" to match this driver? It will not use
> > > > > of_platform_device_create(). It will use of_find_property() to
> > > > > determine whether and nvmem-cells can be used. If there is no
> > > > > nvmem-cells,
> > > > it will use the old way, which supports old DTBS. There is no need
> > > > to add new compatible.
> > > >
> > > > No, the soc@0 is not a proper name for the driver.
> > >
> > > I have no good idea, please give suggestion. Should I still add new compatible?
> > > Should I still keep device_initcall? If use
> > > of_platform_device_create(), which node should I use?
> > 
> > I mentioned my idea in the email before - of_platform_device_create() to bind
> > to the soc node. This will have to be in the initcall, you don't have a choice to
> > avoid it, since there was no compatible before.
> >
> 
> 	node = of_find_node_by_path("/soc@0");
> 	if (!node)
> 		return -ENODEV;
> 
> 	pdev = of_platform_device_create(node, "XXX", NULL);
> 	if (!pdev)
> 		return -ENODEV;
> 
> Cannot use of_platform_device_create because "of_node_test_and_set_flag(np, OF_POPULATED)" returns true.
> of_platform_device_create is used to create platform device, but soc@0 is created by common code. I don't know how
> to bind to the soc node. The way I did in v3 seems not bad, it can work correctly and support old DTBs. Can I keep this way?

Indeed, it would require some more hacks and actually might not work at
all since bus device is already created. Keep the old way and fix other
pointed out issues.

Best regards,
Krzysztof