diff mbox series

[v4,4/4] soc: imx8m: change to use platform driver

Message ID 20201120101112.31819-4-alice.guo@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID | expand

Commit Message

Alice Guo Nov. 20, 2020, 10:11 a.m. UTC 
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.

v2: remove the subject prefix "LF-2571-4"
v3: Keep the original way which uses device_initcall to read soc unique
    ID, and add the other way which uses module_platform_driver and
    nvmem API, so that it will not break the old version DTBs.
v4: delete "__maybe_unused"
    delete MODULE_DEVICE_TABLE(of, imx8m_soc_match);
    rename match table, "fsl,imx8mm/n/q/p" is actually a machine
compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0
    delete "flag" and change to determine whether the pointer is NULL
    ues of_find_matching_node_and_match()
    delete of_match_ptr()

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

--
2.17.1

Comments

Krzysztof Kozlowski Nov. 20, 2020, 10:46 a.m. UTC | #1 
On Fri, Nov 20, 2020 at 06:11:12PM +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.
> 
> v2: remove the subject prefix "LF-2571-4"
> v3: Keep the original way which uses device_initcall to read soc unique
>     ID, and add the other way which uses module_platform_driver and
>     nvmem API, so that it will not break the old version DTBs.
> v4: delete "__maybe_unused"
>     delete MODULE_DEVICE_TABLE(of, imx8m_soc_match);
>     rename match table, "fsl,imx8mm/n/q/p" is actually a machine
> compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0
>     delete "flag" and change to determine whether the pointer is NULL
>     ues of_find_matching_node_and_match()
>     delete of_match_ptr()

Put all the patch version changelogs after --- separator, so they will
not go to the commit.

> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
>  drivers/soc/imx/soc-imx8m.c | 85 +++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> index cc57a384d74d..1b0a34e545ae 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);
>  };
> 
>  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)
>  {
>  	struct device_node *np;
>  	void __iomem *ocotp_base;
> @@ -75,9 +77,19 @@ 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 (dev) {
> +		int ret = 0;
> +
> +		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> +		if (ret) {
> +			iounmap(ocotp_base);

What about other cleanup parts? of_node_put?

> +			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 +119,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)
>  {
>  	struct device_node *np;
>  	void __iomem *anatop_base;
> @@ -125,7 +137,15 @@ static u32 __init imx8mm_soc_revision(void)
>  	iounmap(anatop_base);
>  	of_node_put(np);
> 
> -	imx8mm_soc_uid();
> +	if (dev) {
> +		int ret = 0;
> +
> +		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> +		if (ret)
> +			return ret;
> +	} else {
> +		imx8mm_soc_uid();
> +	}
> 
>  	return rev;
>  }
> @@ -150,7 +170,7 @@ static const struct imx8_soc_data imx8mp_soc_data = {
>  	.soc_revision = imx8mm_soc_revision,
>  };
> 
> -static __maybe_unused const struct of_device_id imx8_soc_match[] = {
> +static const struct of_device_id imx8_machine_match[] = {
>  	{ .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
>  	{ .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
>  	{ .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, },
> @@ -158,12 +178,20 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = {
>  	{ }
>  };
> 
> +static const struct of_device_id imx8_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, },
> +	{ }
> +};
> +
>  #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_info(struct platform_device *pdev)
>  {
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
> @@ -182,7 +210,10 @@ static int __init imx8_soc_init(void)
>  	if (ret)
>  		goto free_soc;
> 
> -	id = of_match_node(imx8_soc_match, of_root);
> +	if (pdev)
> +		id = of_match_node(imx8_soc_match, pdev->dev.of_node);
> +	else
> +		id = of_match_node(imx8_machine_match, of_root);
>  	if (!id) {
>  		ret = -ENODEV;
>  		goto free_soc;
> @@ -191,8 +222,16 @@ static int __init imx8_soc_init(void)
>  	data = id->data;
>  	if (data) {
>  		soc_dev_attr->soc_id = data->name;
> -		if (data->soc_revision)
> -			soc_rev = data->soc_revision();
> +		if (data->soc_revision) {
> +			if (pdev) {
> +				soc_rev = data->soc_revision(&pdev->dev);
> +				ret = soc_rev;
> +				if (ret < 0)
> +					goto free_soc;
> +			} else {
> +				soc_rev = data->soc_revision(NULL);
> +			}
> +		}
>  	}
> 
>  	soc_dev_attr->revision = imx8_revision(soc_rev);
> @@ -230,4 +269,26 @@ static int __init imx8_soc_init(void)
>  	kfree(soc_dev_attr);
>  	return ret;
>  }
> +
> +static int __init imx8_soc_init(void)
> +{
> +	int ret = 0;

Skip the initialization.

You need to document why the initcal is still there. Please write that
it is purely for backward compatibility.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
index cc57a384d74d..1b0a34e545ae 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);
 };

 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)
 {
 	struct device_node *np;
 	void __iomem *ocotp_base;
@@ -75,9 +77,19 @@  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 (dev) {
+		int ret = 0;
+
+		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
+		if (ret) {
+			iounmap(ocotp_base);
+			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 +119,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)
 {
 	struct device_node *np;
 	void __iomem *anatop_base;
@@ -125,7 +137,15 @@  static u32 __init imx8mm_soc_revision(void)
 	iounmap(anatop_base);
 	of_node_put(np);

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

 	return rev;
 }
@@ -150,7 +170,7 @@  static const struct imx8_soc_data imx8mp_soc_data = {
 	.soc_revision = imx8mm_soc_revision,
 };

-static __maybe_unused const struct of_device_id imx8_soc_match[] = {
+static const struct of_device_id imx8_machine_match[] = {
 	{ .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
 	{ .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
 	{ .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, },
@@ -158,12 +178,20 @@  static __maybe_unused const struct of_device_id imx8_soc_match[] = {
 	{ }
 };

+static const struct of_device_id imx8_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, },
+	{ }
+};
+
 #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_info(struct platform_device *pdev)
 {
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
@@ -182,7 +210,10 @@  static int __init imx8_soc_init(void)
 	if (ret)
 		goto free_soc;

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

 	soc_dev_attr->revision = imx8_revision(soc_rev);
@@ -230,4 +269,26 @@  static int __init imx8_soc_init(void)
 	kfree(soc_dev_attr);
 	return ret;
 }
+
+static int __init imx8_soc_init(void)
+{
+	int ret = 0;
+
+	if (of_find_matching_node_and_match(NULL, imx8_soc_match, NULL))
+		return 0;
+
+	ret = imx8_soc_info(NULL);
+	return ret;
+}
 device_initcall(imx8_soc_init);
+
+static struct platform_driver imx8_soc_info_driver = {
+	.probe = imx8_soc_info,
+	.driver = {
+		.name = "imx8_soc_info",
+		.of_match_table = imx8_soc_match,
+	},
+};
+
+module_platform_driver(imx8_soc_info_driver);
+MODULE_LICENSE("GPL v2");