[PATCH V6 1/2] soc: imx: Add SCU SoC info driver support

[PATCH V6 1/2] soc: imx: Add SCU SoC info driver support

[Anson Huang]

Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce
driver dependency into Kconfig as CONFIG_IMX_SCU must be
selected to support i.MX SCU SoC driver, also need to use
platform driver model to make sure IMX_SCU driver is probed
before i.MX SCU SoC driver.

With this patch, SoC info can be read from sysfs:

i.mx8qxp-mek# cat /sys/devices/soc0/family
Freescale i.MX

i.mx8qxp-mek# cat /sys/devices/soc0/soc_id
0x2

i.mx8qxp-mek# cat /sys/devices/soc0/machine
Freescale i.MX8QXP MEK

i.mx8qxp-mek# cat /sys/devices/soc0/revision
1.1

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
Changes since V5:
	- remove unnecessary comment;
	- improve the IPC message structure definition;
	- improve the error check and return value in imx_scu_soc_init() to save some code.
---
 drivers/soc/imx/Kconfig       |   9 +++
 drivers/soc/imx/Makefile      |   1 +
 drivers/soc/imx/soc-imx-scu.c | 147 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 drivers/soc/imx/soc-imx-scu.c

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index ade1b46..8aaebf1 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig
@@ -8,4 +8,13 @@ config IMX_GPCV2_PM_DOMAINS
 	select PM_GENERIC_DOMAINS
 	default y if SOC_IMX7D
 
+config IMX_SCU_SOC
+	bool "i.MX System Controller Unit SoC info support"
+	depends on IMX_SCU
+	select SOC_BUS
+	help
+	  If you say yes here you get support for the NXP i.MX System
+	  Controller Unit SoC info module, it will provide the SoC info
+	  like SoC family, ID and revision etc.
+
 endmenu
diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index caa8653..cf9ca42 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
 obj-$(CONFIG_ARCH_MXC) += soc-imx8.o
+obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
new file mode 100644
index 0000000..258c987
--- /dev/null
+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <dt-bindings/firmware/imx/rsrc.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#define IMX_SCU_SOC_DRIVER_NAME		"imx-scu-soc"
+
+static struct imx_sc_ipc *soc_ipc_handle;
+
+struct imx_sc_msg_misc_get_soc_id {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct {
+			u32 control;
+			u16 resource;
+		} __packed req;
+		struct {
+			u32 id;
+		} __packed resp;
+	} data;
+} __packed;
+
+static u32 imx_scu_soc_id(void)
+{
+	struct imx_sc_msg_misc_get_soc_id msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_MISC;
+	hdr->func = IMX_SC_MISC_FUNC_GET_CONTROL;
+	hdr->size = 3;
+
+	msg.data.req.control = IMX_SC_C_ID;
+	msg.data.req.resource = IMX_SC_R_SYSTEM;
+
+	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
+	if (ret) {
+		pr_err("%s: get soc info failed, ret %d\n", __func__, ret);
+		/* return 0 means getting revision failed */
+		return 0;
+	}
+
+	return msg.data.resp.id;
+}
+
+static int imx_scu_soc_probe(struct platform_device *pdev)
+{
+	struct soc_device_attribute *soc_dev_attr;
+	struct soc_device *soc_dev;
+	u32 id, val;
+	int ret;
+
+	ret = imx_scu_get_handle(&soc_ipc_handle);
+	if (ret)
+		return ret;
+
+	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
+				    GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	soc_dev_attr->family = "Freescale i.MX";
+
+	ret = of_property_read_string(of_root,
+				      "model",
+				      &soc_dev_attr->machine);
+	if (ret)
+		return ret;
+
+	id = imx_scu_soc_id();
+
+	/* format soc_id value passed from SCU firmware */
+	val = id & 0x1f;
+	soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
+			       : "unknown";
+	if (!soc_dev_attr->soc_id)
+		return -ENOMEM;
+
+	/* format revision value passed from SCU firmware */
+	val = (id >> 5) & 0xf;
+	val = (((val >> 2) + 1) << 4) | (val & 0x3);
+	soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
+						 "%d.%d",
+						 (val >> 4) & 0xf,
+						 val & 0xf) : "unknown";
+	if (!soc_dev_attr->revision) {
+		ret = -ENOMEM;
+		goto free_soc_id;
+	}
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		ret = PTR_ERR(soc_dev);
+		goto free_revision;
+	}
+
+	return 0;
+
+free_revision:
+	kfree(soc_dev_attr->revision);
+free_soc_id:
+	kfree(soc_dev_attr->soc_id);
+	return ret;
+}
+
+static struct platform_driver imx_scu_soc_driver = {
+	.driver = {
+		.name = IMX_SCU_SOC_DRIVER_NAME,
+	},
+	.probe = imx_scu_soc_probe,
+};
+
+static int __init imx_scu_soc_init(void)
+{
+	struct platform_device *imx_scu_soc_pdev;
+	struct device_node *np;
+	int ret;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
+	if (!np)
+		return -ENODEV;
+
+	of_node_put(np);
+
+	ret = platform_driver_register(&imx_scu_soc_driver);
+	if (ret)
+		return ret;
+
+	imx_scu_soc_pdev =
+		platform_device_register_simple(IMX_SCU_SOC_DRIVER_NAME,
+						-1,
+						NULL,
+						0);
+	if (IS_ERR(imx_scu_soc_pdev))
+		platform_driver_unregister(&imx_scu_soc_driver);
+
+	return PTR_ERR_OR_ZERO(imx_scu_soc_pdev);
+}
+device_initcall(imx_scu_soc_init);
-- 
2.7.4

[PATCH V6 2/2] arm64: defconfig: Add i.MX SCU SoC info driver

[Anson Huang]

This patch selects CONFIG_IMX_SCU_SOC by default to support
i.MX system controller unit SoC info driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
No changes.
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c91642d..aaca4a0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -694,6 +694,7 @@ CONFIG_RPMSG_QCOM_GLINK_RPM=y
 CONFIG_RPMSG_QCOM_GLINK_SMEM=m
 CONFIG_RPMSG_QCOM_SMD=y
 CONFIG_RASPBERRYPI_POWER=y
+CONFIG_IMX_SCU_SOC=y
 CONFIG_QCOM_COMMAND_DB=y
 CONFIG_QCOM_GENI_SE=y
 CONFIG_QCOM_GLINK_SSR=m
-- 
2.7.4

Re: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support

[Russell King - ARM Linux admin]

On Thu, May 23, 2019 at 04:53:46AM +0000, Anson Huang wrote:
> Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce
> driver dependency into Kconfig as CONFIG_IMX_SCU must be
> selected to support i.MX SCU SoC driver, also need to use
> platform driver model to make sure IMX_SCU driver is probed
> before i.MX SCU SoC driver.

This seems buggy...

> +static int imx_scu_soc_probe(struct platform_device *pdev)
> +{
> +	struct soc_device_attribute *soc_dev_attr;
> +	struct soc_device *soc_dev;
> +	u32 id, val;
> +	int ret;
> +
> +	ret = imx_scu_get_handle(&soc_ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> +				    GFP_KERNEL);
> +	if (!soc_dev_attr)
> +		return -ENOMEM;
> +
> +	soc_dev_attr->family = "Freescale i.MX";
> +
> +	ret = of_property_read_string(of_root,
> +				      "model",
> +				      &soc_dev_attr->machine);
> +	if (ret)
> +		return ret;
> +
> +	id = imx_scu_soc_id();
> +
> +	/* format soc_id value passed from SCU firmware */
> +	val = id & 0x1f;
> +	soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
> +			       : "unknown";

soc_id can either be an allocated string, or it can be a static string
of "unknown".

> +	if (!soc_dev_attr->soc_id)
> +		return -ENOMEM;
> +
> +	/* format revision value passed from SCU firmware */
> +	val = (id >> 5) & 0xf;
> +	val = (((val >> 2) + 1) << 4) | (val & 0x3);
> +	soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
> +						 "%d.%d",
> +						 (val >> 4) & 0xf,
> +						 val & 0xf) : "unknown";

revision here is the same.

> +	if (!soc_dev_attr->revision) {
> +		ret = -ENOMEM;
> +		goto free_soc_id;
> +	}
> +
> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev)) {
> +		ret = PTR_ERR(soc_dev);
> +		goto free_revision;
> +	}
> +
> +	return 0;
> +
> +free_revision:
> +	kfree(soc_dev_attr->revision);
> +free_soc_id:
> +	kfree(soc_dev_attr->soc_id);

Yet here you blindly kfree(), which means you could be doing in effect
kfree("unknown") - which will likely result in a kernel oops.

Either always make revision/soc_id an allocated string, or use some
static storage for the strings (they're only small, static storage is
likely to be much more efficient in this case, and there can only be
one of these in any case.)

> +	return ret;
> +}
> +
> +static struct platform_driver imx_scu_soc_driver = {
> +	.driver = {
> +		.name = IMX_SCU_SOC_DRIVER_NAME,
> +	},
> +	.probe = imx_scu_soc_probe,
> +};
> +
> +static int __init imx_scu_soc_init(void)
> +{
> +	struct platform_device *imx_scu_soc_pdev;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
> +	if (!np)
> +		return -ENODEV;
> +
> +	of_node_put(np);
> +
> +	ret = platform_driver_register(&imx_scu_soc_driver);
> +	if (ret)
> +		return ret;
> +
> +	imx_scu_soc_pdev =
> +		platform_device_register_simple(IMX_SCU_SOC_DRIVER_NAME,
> +						-1,
> +						NULL,
> +						0);
> +	if (IS_ERR(imx_scu_soc_pdev))
> +		platform_driver_unregister(&imx_scu_soc_driver);
> +
> +	return PTR_ERR_OR_ZERO(imx_scu_soc_pdev);
> +}
> +device_initcall(imx_scu_soc_init);
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support

[Anson Huang]

Hi, Russell

> -----Original Message-----
> From: Russell King - ARM Linux admin [mailto:linux@armlinux.org.uk]
> Sent: Thursday, May 23, 2019 5:10 PM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: catalin.marinas@arm.com; will.deacon@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; maxime.ripard@bootlin.com; olof@lixom.net;
> agross@kernel.org; jagan@amarulasolutions.com;
> bjorn.andersson@linaro.org; Leonard Crestez <leonard.crestez@nxp.com>;
> dinguyen@kernel.org; enric.balletbo@collabora.com; Aisheng Dong
> <aisheng.dong@nxp.com>; Abel Vesa <abel.vesa@nxp.com>;
> l.stach@pengutronix.de; tglx@linutronix.de; robh@kernel.org;
> gregkh@linuxfoundation.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support
> 
> On Thu, May 23, 2019 at 04:53:46AM +0000, Anson Huang wrote:
> > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce driver
> > dependency into Kconfig as CONFIG_IMX_SCU must be selected to support
> > i.MX SCU SoC driver, also need to use platform driver model to make
> > sure IMX_SCU driver is probed before i.MX SCU SoC driver.
> 
> This seems buggy...

We have to use platform driver model, but there is no such soc device in DT file,
so we created the platform device in this driver directly.

> 
> > +static int imx_scu_soc_probe(struct platform_device *pdev) {
> > +	struct soc_device_attribute *soc_dev_attr;
> > +	struct soc_device *soc_dev;
> > +	u32 id, val;
> > +	int ret;
> > +
> > +	ret = imx_scu_get_handle(&soc_ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> > +				    GFP_KERNEL);
> > +	if (!soc_dev_attr)
> > +		return -ENOMEM;
> > +
> > +	soc_dev_attr->family = "Freescale i.MX";
> > +
> > +	ret = of_property_read_string(of_root,
> > +				      "model",
> > +				      &soc_dev_attr->machine);
> > +	if (ret)
> > +		return ret;
> > +
> > +	id = imx_scu_soc_id();
> > +
> > +	/* format soc_id value passed from SCU firmware */
> > +	val = id & 0x1f;
> > +	soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
> > +			       : "unknown";
> 
> soc_id can either be an allocated string, or it can be a static string of
> "unknown".
> 
> > +	if (!soc_dev_attr->soc_id)
> > +		return -ENOMEM;
> > +
> > +	/* format revision value passed from SCU firmware */
> > +	val = (id >> 5) & 0xf;
> > +	val = (((val >> 2) + 1) << 4) | (val & 0x3);
> > +	soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
> > +						 "%d.%d",
> > +						 (val >> 4) & 0xf,
> > +						 val & 0xf) : "unknown";
> 
> revision here is the same.
> 
> > +	if (!soc_dev_attr->revision) {
> > +		ret = -ENOMEM;
> > +		goto free_soc_id;
> > +	}
> > +
> > +	soc_dev = soc_device_register(soc_dev_attr);
> > +	if (IS_ERR(soc_dev)) {
> > +		ret = PTR_ERR(soc_dev);
> > +		goto free_revision;
> > +	}
> > +
> > +	return 0;
> > +
> > +free_revision:
> > +	kfree(soc_dev_attr->revision);
> > +free_soc_id:
> > +	kfree(soc_dev_attr->soc_id);
> 
> Yet here you blindly kfree(), which means you could be doing in effect
> kfree("unknown") - which will likely result in a kernel oops.
> 
> Either always make revision/soc_id an allocated string, or use some static
> storage for the strings (they're only small, static storage is likely to be much
> more efficient in this case, and there can only be one of these in any case.)

You are right, actually the "unknown" message is NOT helpful at all, so in next version,
I will just check the return value from SCU firmware, if it fail, just skip this driver probe,
If it success, it will just print out the soc_id and revision passing from SCU firmware, so no
need to have "unknown" string here.

+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -27,7 +27,7 @@ struct imx_sc_msg_misc_get_soc_id {
        } data;
 } __packed;

-static u32 imx_scu_soc_id(void)
+static int imx_scu_soc_id(void)
 {
        struct imx_sc_msg_misc_get_soc_id msg;
        struct imx_sc_rpc_msg *hdr = &msg.hdr;
@@ -44,8 +44,7 @@ static u32 imx_scu_soc_id(void)
        ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
        if (ret) {
                pr_err("%s: get soc info failed, ret %d\n", __func__, ret);
-               /* return 0 means getting revision failed */
-               return 0;
+               return ret;
        }

        return msg.data.resp.id;
@@ -55,7 +54,8 @@ static int imx_scu_soc_probe(struct platform_device *pdev)
 {
        struct soc_device_attribute *soc_dev_attr;
        struct soc_device *soc_dev;
-       u32 id, val;
+       u32 val;
+       int id;
        int ret;

        ret = imx_scu_get_handle(&soc_ipc_handle);
@@ -76,21 +76,22 @@ static int imx_scu_soc_probe(struct platform_device *pdev)
                return ret;

        id = imx_scu_soc_id();
+       if (id < 0)
+               return -EINVAL;

        /* format soc_id value passed from SCU firmware */
        val = id & 0x1f;
-       soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
-                              : "unknown";
+       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", val);
        if (!soc_dev_attr->soc_id)
                return -ENOMEM;

        /* format revision value passed from SCU firmware */
        val = (id >> 5) & 0xf;
        val = (((val >> 2) + 1) << 4) | (val & 0x3);
-       soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
-                                                "%d.%d",
-                                                (val >> 4) & 0xf,
-                                                val & 0xf) : "unknown";
+       soc_dev_attr->revision = kasprintf(GFP_KERNEL,
+                                          "%d.%d",
+                                          (val >> 4) & 0xf,
+                                          val & 0xf);
        if (!soc_dev_attr->revision) {

Thanks,
Anson.

> 
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver imx_scu_soc_driver = {
> > +	.driver = {
> > +		.name = IMX_SCU_SOC_DRIVER_NAME,
> > +	},
> > +	.probe = imx_scu_soc_probe,
> > +};
> > +
> > +static int __init imx_scu_soc_init(void) {
> > +	struct platform_device *imx_scu_soc_pdev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	of_node_put(np);
> > +
> > +	ret = platform_driver_register(&imx_scu_soc_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	imx_scu_soc_pdev =
> > +
> 	platform_device_register_simple(IMX_SCU_SOC_DRIVER_NAME,
> > +						-1,
> > +						NULL,
> > +						0);
> > +	if (IS_ERR(imx_scu_soc_pdev))
> > +		platform_driver_unregister(&imx_scu_soc_driver);
> > +
> > +	return PTR_ERR_OR_ZERO(imx_scu_soc_pdev);
> > +}
> > +device_initcall(imx_scu_soc_init);
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-
> kernel&amp;data=02%7C0
> >
> 1%7Canson.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C
> 686ea1d
> >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&amp;sdat
> a=mXdow
> >
> fLshJ%2BQlyLJ%2BbyYwuDUXh3CwOBAiZ%2BvVED%2FRh4%3D&amp;reserve
> d=0
> >
> 
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> armlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=02%7C01%7Canso
> n.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&amp;sdata
> =rsrH%2F9pqdLZUJ0E%2BWUa2Kf71rtOcNDyjJ81R8Ex6yQs%3D&amp;reserve
> d=0
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down
> 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up