[PATCH] Revert "soc: imx8m: change to use platform driver"

[PATCH] Revert "soc: imx8m: change to use platform driver"

[Lucas Stach]

With the SoC matching changed to a platform driver the match data
is available only after other drivers, which may rely on it are
already probed. This breaks at least the CAAM driver on i.MX8M.
Revert the change until all those drivers have been audited and
changed to be able to eal with match data being available later
in the boot process.

Fixes: 7d981405d0fd ("soc: imx8m: change to use platform driver"")
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/soc-imx8m.c | 84 ++++++-------------------------------
 1 file changed, 12 insertions(+), 72 deletions(-)

diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
index 071e14496e4b..cc57a384d74d 100644
--- a/drivers/soc/imx/soc-imx8m.c
+++ b/drivers/soc/imx/soc-imx8m.c
@@ -5,8 +5,6 @@
 
 #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>
@@ -31,7 +29,7 @@
 
 struct imx8_soc_data {
 	char *name;
-	u32 (*soc_revision)(struct device *dev);
+	u32 (*soc_revision)(void);
 };
 
 static u64 soc_uid;
@@ -52,7 +50,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(struct device *dev)
+static u32 __init imx8mq_soc_revision(void)
 {
 	struct device_node *np;
 	void __iomem *ocotp_base;
@@ -77,20 +75,9 @@ static u32 __init imx8mq_soc_revision(struct device *dev)
 			rev = REV_B1;
 	}
 
-	if (dev) {
-		int ret;
-
-		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
-		if (ret) {
-			iounmap(ocotp_base);
-			of_node_put(np);
-			return ret;
-		}
-	} else {
-		soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
-		soc_uid <<= 32;
-		soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
-	}
+	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);
@@ -120,7 +107,7 @@ static void __init imx8mm_soc_uid(void)
 	of_node_put(np);
 }
 
-static u32 __init imx8mm_soc_revision(struct device *dev)
+static u32 __init imx8mm_soc_revision(void)
 {
 	struct device_node *np;
 	void __iomem *anatop_base;
@@ -138,15 +125,7 @@ static u32 __init imx8mm_soc_revision(struct device *dev)
 	iounmap(anatop_base);
 	of_node_put(np);
 
-	if (dev) {
-		int ret;
-
-		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
-		if (ret)
-			return ret;
-	} else {
-		imx8mm_soc_uid();
-	}
+	imx8mm_soc_uid();
 
 	return rev;
 }
@@ -171,7 +150,7 @@ static const struct imx8_soc_data imx8mp_soc_data = {
 	.soc_revision = imx8mm_soc_revision,
 };
 
-static __maybe_unused const struct of_device_id imx8_machine_match[] = {
+static __maybe_unused const struct of_device_id imx8_soc_match[] = {
 	{ .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
 	{ .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
 	{ .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, },
@@ -179,20 +158,12 @@ static __maybe_unused const struct of_device_id imx8_machine_match[] = {
 	{ }
 };
 
-static __maybe_unused 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 imx8_soc_info(struct platform_device *pdev)
+static int __init imx8_soc_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
@@ -211,10 +182,7 @@ static int imx8_soc_info(struct platform_device *pdev)
 	if (ret)
 		goto free_soc;
 
-	if (pdev)
-		id = of_match_node(imx8_soc_match, pdev->dev.of_node);
-	else
-		id = of_match_node(imx8_machine_match, of_root);
+	id = of_match_node(imx8_soc_match, of_root);
 	if (!id) {
 		ret = -ENODEV;
 		goto free_soc;
@@ -223,16 +191,8 @@ static int imx8_soc_info(struct platform_device *pdev)
 	data = id->data;
 	if (data) {
 		soc_dev_attr->soc_id = data->name;
-		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);
-			}
-		}
+		if (data->soc_revision)
+			soc_rev = data->soc_revision();
 	}
 
 	soc_dev_attr->revision = imx8_revision(soc_rev);
@@ -270,24 +230,4 @@ static int imx8_soc_info(struct platform_device *pdev)
 	kfree(soc_dev_attr);
 	return ret;
 }
-
-/* Retain device_initcall is for backward compatibility with DTS. */
-static int __init imx8_soc_init(void)
-{
-	if (of_find_matching_node_and_match(NULL, imx8_soc_match, NULL))
-		return 0;
-
-	return imx8_soc_info(NULL);
-}
 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");
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "soc: imx8m: change to use platform driver"

[Frieder Schrempf]

On 28.06.21 23:09, Lucas Stach wrote:
> With the SoC matching changed to a platform driver the match data
> is available only after other drivers, which may rely on it are
> already probed. This breaks at least the CAAM driver on i.MX8M.
> Revert the change until all those drivers have been audited and
> changed to be able to eal with match data being available later
> in the boot process.
> 
> Fixes: 7d981405d0fd ("soc: imx8m: change to use platform driver"")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

I didn't follow all of the history of this, but there was a previous fix that worked for me [1] and was never merged (probably for some reason).

This revert also works for me and fixes the CAAM crash at boot.

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210429140250.2321-1-alice.guo@oss.nxp.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] Revert "soc: imx8m: change to use platform driver"

[Peng Fan (OSS)]

> Subject: [PATCH] Revert "soc: imx8m: change to use platform driver"
> 
> With the SoC matching changed to a platform driver the match data is
> available only after other drivers, which may rely on it are already probed.
> This breaks at least the CAAM driver on i.MX8M.
> Revert the change until all those drivers have been audited and changed to be
> able to eal with match data being available later in the boot process.
> 
> Fixes: 7d981405d0fd ("soc: imx8m: change to use platform driver"")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Acked-by: Peng Fan <peng.fan@nxp.com>

BTW: we need a better way to support kexec.

Regards,
Peng.

> ---
>  drivers/soc/imx/soc-imx8m.c | 84 ++++++-------------------------------
>  1 file changed, 12 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> index 071e14496e4b..cc57a384d74d 100644
> --- a/drivers/soc/imx/soc-imx8m.c
> +++ b/drivers/soc/imx/soc-imx8m.c
> @@ -5,8 +5,6 @@
> 
>  #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>
> @@ -31,7 +29,7 @@
> 
>  struct imx8_soc_data {
>  	char *name;
> -	u32 (*soc_revision)(struct device *dev);
> +	u32 (*soc_revision)(void);
>  };
> 
>  static u64 soc_uid;
> @@ -52,7 +50,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(struct device *dev)
> +static u32 __init imx8mq_soc_revision(void)
>  {
>  	struct device_node *np;
>  	void __iomem *ocotp_base;
> @@ -77,20 +75,9 @@ static u32 __init imx8mq_soc_revision(struct device
> *dev)
>  			rev = REV_B1;
>  	}
> 
> -	if (dev) {
> -		int ret;
> -
> -		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> -		if (ret) {
> -			iounmap(ocotp_base);
> -			of_node_put(np);
> -			return ret;
> -		}
> -	} else {
> -		soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> -		soc_uid <<= 32;
> -		soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> -	}
> +	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);
> @@ -120,7 +107,7 @@ static void __init imx8mm_soc_uid(void)
>  	of_node_put(np);
>  }
> 
> -static u32 __init imx8mm_soc_revision(struct device *dev)
> +static u32 __init imx8mm_soc_revision(void)
>  {
>  	struct device_node *np;
>  	void __iomem *anatop_base;
> @@ -138,15 +125,7 @@ static u32 __init imx8mm_soc_revision(struct
> device *dev)
>  	iounmap(anatop_base);
>  	of_node_put(np);
> 
> -	if (dev) {
> -		int ret;
> -
> -		ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> -		if (ret)
> -			return ret;
> -	} else {
> -		imx8mm_soc_uid();
> -	}
> +	imx8mm_soc_uid();
> 
>  	return rev;
>  }
> @@ -171,7 +150,7 @@ static const struct imx8_soc_data imx8mp_soc_data
> = {
>  	.soc_revision = imx8mm_soc_revision,
>  };
> 
> -static __maybe_unused const struct of_device_id imx8_machine_match[] = {
> +static __maybe_unused const struct of_device_id imx8_soc_match[] = {
>  	{ .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
>  	{ .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
>  	{ .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, }, @@ -179,20
> +158,12 @@ static __maybe_unused const struct of_device_id
> imx8_machine_match[] = {
>  	{ }
>  };
> 
> -static __maybe_unused 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 imx8_soc_info(struct platform_device *pdev)
> +static int __init imx8_soc_init(void)
>  {
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
> @@ -211,10 +182,7 @@ static int imx8_soc_info(struct platform_device
> *pdev)
>  	if (ret)
>  		goto free_soc;
> 
> -	if (pdev)
> -		id = of_match_node(imx8_soc_match, pdev->dev.of_node);
> -	else
> -		id = of_match_node(imx8_machine_match, of_root);
> +	id = of_match_node(imx8_soc_match, of_root);
>  	if (!id) {
>  		ret = -ENODEV;
>  		goto free_soc;
> @@ -223,16 +191,8 @@ static int imx8_soc_info(struct platform_device
> *pdev)
>  	data = id->data;
>  	if (data) {
>  		soc_dev_attr->soc_id = data->name;
> -		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);
> -			}
> -		}
> +		if (data->soc_revision)
> +			soc_rev = data->soc_revision();
>  	}
> 
>  	soc_dev_attr->revision = imx8_revision(soc_rev); @@ -270,24 +230,4
> @@ static int imx8_soc_info(struct platform_device *pdev)
>  	kfree(soc_dev_attr);
>  	return ret;
>  }
> -
> -/* Retain device_initcall is for backward compatibility with DTS. */ -static int
> __init imx8_soc_init(void) -{
> -	if (of_find_matching_node_and_match(NULL, imx8_soc_match, NULL))
> -		return 0;
> -
> -	return imx8_soc_info(NULL);
> -}
>  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");
> --
> 2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "soc: imx8m: change to use platform driver"

[Shawn Guo]

On Mon, Jun 28, 2021 at 11:09:55PM +0200, Lucas Stach wrote:
> With the SoC matching changed to a platform driver the match data
> is available only after other drivers, which may rely on it are
> already probed. This breaks at least the CAAM driver on i.MX8M.
> Revert the change until all those drivers have been audited and
> changed to be able to eal with match data being available later
> in the boot process.
> 
> Fixes: 7d981405d0fd ("soc: imx8m: change to use platform driver"")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel