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

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
i.MX8QXP

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 <redacted>
---
Changes since V2:
	- using device_initcall instead of module_init;
	- check of_match_node in init function and ONLY register platform driver when matched, this
	  is to avoid unnecessary probe for non SCU based i.MX8 SoCs.
---
 drivers/soc/imx/Kconfig       |   9 +++
 drivers/soc/imx/Makefile      |   1 +
 drivers/soc/imx/soc-imx-scu.c | 173 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 drivers/soc/imx/soc-imx-scu.c

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index d80f899..cbc1a41 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig

@@ -7,4 +7,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 d6b529e0..ddf343d 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile

@@ -1,3 +1,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..243c418
--- /dev/null
+++ b/drivers/soc/imx/soc-imx-scu.c

@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <dt-bindings/firmware/imx/rsrc.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/module.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;
+static struct platform_device *imx_scu_soc_pdev;
+
+struct imx_sc_msg_misc_get_soc_id {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct {
+			u32 control;
+			u16 resource;
+		} send;
+		struct {
+			u32 id;
+			u16 reserved;
+		} resp;
+	} data;
+} __packed;
+
+struct imx_scu_soc_data {
+	char *name;
+	u32 (*soc_revision)(void);
+};
+
+static u32 imx8qxp_soc_revision(void)
+{
+	struct imx_sc_msg_misc_get_soc_id msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	u32 rev;
+	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.send.control = IMX_SC_C_ID;
+	msg.data.send.resource = IMX_SC_R_SYSTEM;
+
+	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
+	if (ret) {
+		dev_err(&imx_scu_soc_pdev->dev,
+			"get soc info failed, ret %d\n", ret);
+		/* return 0 means getting revision failed */
+		return 0;
+	}
+
+	/* format revision value passed from SCU firmware */
+	rev = (msg.data.resp.id >> 5) & 0xf;
+	rev = (((rev >> 2) + 1) << 4) | (rev & 0x3);
+
+	return rev;
+}
+
+static const struct imx_scu_soc_data imx8qxp_soc_data = {
+	.name = "i.MX8QXP",
+	.soc_revision = imx8qxp_soc_revision,
+};
+
+static const struct of_device_id imx_scu_soc_match[] = {
+	{ .compatible = "fsl,imx8qxp", .data = &imx8qxp_soc_data, },
+	{ }
+};
+
+#define imx_scu_revision(soc_rev) \
+	soc_rev ? \
+	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev & 0xf) : \
+	"unknown"
+
+static int imx_scu_soc_probe(struct platform_device *pdev)
+{
+	struct soc_device_attribute *soc_dev_attr;
+	const struct imx_scu_soc_data *data;
+	const struct of_device_id *id;
+	struct soc_device *soc_dev;
+	u32 soc_rev = 0;
+	int ret;
+
+	/* wait i.MX SCU driver ready */
+	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(pdev->dev.of_node,
+				      "model",
+				      &soc_dev_attr->machine);
+	if (ret)
+		return ret;
+
+	id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
+	data = id->data;
+	if (data) {
+		soc_dev_attr->soc_id = data->name;
+		if (data->soc_revision)
+			soc_rev = data->soc_revision();
+	}
+
+	soc_dev_attr->revision = imx_scu_revision(soc_rev);
+	if (!soc_dev_attr->revision)
+		return -ENODEV;
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr->revision);
+		return PTR_ERR(soc_dev);
+	}
+
+	return 0;
+}
+
+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)
+{
+	const struct of_device_id *id;
+	struct device_node *root;
+	int ret;
+
+	root = of_find_node_by_path("/");
+	id = of_match_node(imx_scu_soc_match, root);
+	if (!id) {
+		of_node_put(root);
+		return -ENODEV;
+	}
+
+	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)) {
+		ret = PTR_ERR(imx_scu_soc_pdev);
+		goto unreg_platform_driver;
+	}
+
+	imx_scu_soc_pdev->dev.of_node = root;
+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&imx_scu_soc_driver);
+	return ret;
+}
+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

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

Signed-off-by: Anson Huang <redacted>
---
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 8871cf7..d3a4508 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig

@@ -691,6 +691,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


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

On 16.05.2019 06:24, 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.

The imx_scu_revision macro returns either kasprintf or "unknown", never 
NULL. So it's not clear what this return -ENODEV does exactly.

It makes more sense to return -ENODEV if get_soc_revision fails, so 
maybe check "soc_rev != 0" instead?

If you really want to check the kasprintf result then you should return 
-ENOMEM for it. It would be clearer if you dropped the imx_scu_revision 
revision macro and open-coded instead.

--
Regards,
Leonard

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

Hi, Leonard

-----Original Message-----
From: Leonard Crestez
Sent: Thursday, May 16, 2019 6:07 PM
To: Anson Huang <redacted>; shawnguo@kernel.org
Cc: catalin.marinas@arm.com; will.deacon@arm.com;
s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
agross@kernel.org; maxime.ripard@bootlin.com; olof@lixom.net;
horms+renesas@verge.net.au; jagan@amarulasolutions.com;
bjorn.andersson@linaro.org; marc.w.gonzalez@free.fr;
dinguyen@kernel.org; enric.balletbo@collabora.com;
l.stach@pengutronix.de; Aisheng Dong [off-list ref]; Abel Vesa
[off-list ref]; robh@kernel.org; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
[off-list ref]
Subject: Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support

On 16.05.2019 06:24, Anson Huang wrote:

quoted

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.

quoted

+#define imx_scu_revision(soc_rev) \
+	soc_rev ? \
+	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &

0xf) : \

quoted

+	"unknown"

quoted

+	id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
+	data = id->data;
+	if (data) {
+		soc_dev_attr->soc_id = data->name;
+		if (data->soc_revision)
+			soc_rev = data->soc_revision();
+	}
+
+	soc_dev_attr->revision = imx_scu_revision(soc_rev);
+	if (!soc_dev_attr->revision)
+		return -ENODEV;

The imx_scu_revision macro returns either kasprintf or "unknown", never
NULL. So it's not clear what this return -ENODEV does exactly.

The kasprintf could return NULL though.

It makes more sense to return -ENODEV if get_soc_revision fails, so maybe
check "soc_rev != 0" instead?

If you really want to check the kasprintf result then you should return -
ENOMEM for it. It would be clearer if you dropped the imx_scu_revision
revision macro and open-coded instead.

This makes more sense, I think maybe we can remove the imx_scu_revision macro,
just use below code instead, and return -ENOMEM if kasprintf returns NULL.

113         soc_dev_attr->revision = soc_rev ? kasprintf(GFP_KERNEL,
114                                                      "%d.%d",
115                                                      (soc_rev >> 4) & 0xf,
116                                                      soc_rev & 0xf) : "unknown";
117         if (!soc_dev_attr->revision)
118                 return -ENOMEM;

BTW, the soc-imx8.c looks like also having same issue, do you think we should fix it
as well?

Anson

--
Regards,
Leonard

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

This is still follow a generic soc driver design.
I wonder if this is still needed after separate scu soc driver from imx8m.
If not needed, this driver probably could be simplied a lot.

Use of_find_compatible_node instead.
Then you remove two unnecessary local variable.

BTW, probably a better way is to match "fsl,imx-scu"?

Please double check if we really need a global imx_scu_soc_pdev.

Regards
Dong Aisheng

+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&imx_scu_soc_driver);
+	return ret;
+}
+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

-----Original Message-----
From: Aisheng Dong
Sent: Thursday, May 16, 2019 7:13 PM
To: Anson Huang <redacted>; catalin.marinas@arm.com;
will.deacon@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
kernel@pengutronix.de; festevam@gmail.com; agross@kernel.org;
maxime.ripard@bootlin.com; olof@lixom.net; horms+renesas@verge.net.au;
jagan@amarulasolutions.com; bjorn.andersson@linaro.org; Leonard Crestez
[off-list ref]; marc.w.gonzalez@free.fr;
dinguyen@kernel.org; enric.balletbo@collabora.com;
l.stach@pengutronix.de; Abel Vesa [off-list ref]; robh@kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Cc: dl-linux-imx <redacted>
Subject: RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support

quoted

From: Anson Huang
Sent: Thursday, May 16, 2019 11:25 AM

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 i.MX8QXP

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 <redacted>
---
Changes since V2:
	- using device_initcall instead of module_init;
	- check of_match_node in init function and ONLY register platform
driver when matched, this
	  is to avoid unnecessary probe for non SCU based i.MX8 SoCs.
---
 drivers/soc/imx/Kconfig       |   9 +++
 drivers/soc/imx/Makefile      |   1 +
 drivers/soc/imx/soc-imx-scu.c | 173
++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 drivers/soc/imx/soc-imx-scu.c

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index

d80f899..cbc1a41 100644

--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig

@@ -7,4 +7,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

d6b529e0..ddf343d 100644

--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile

@@ -1,3 +1,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..243c418

--- /dev/null
+++ b/drivers/soc/imx/soc-imx-scu.c

@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <dt-bindings/firmware/imx/rsrc.h> #include
+<linux/firmware/imx/sci.h> #include <linux/module.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; static struct
+platform_device *imx_scu_soc_pdev;
+
+struct imx_sc_msg_misc_get_soc_id {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct {
+			u32 control;
+			u16 resource;
+		} send;
+		struct {
+			u32 id;
+			u16 reserved;
+		} resp;
+	} data;
+} __packed;
+
+struct imx_scu_soc_data {
+	char *name;
+	u32 (*soc_revision)(void);
+};
+
+static u32 imx8qxp_soc_revision(void) {
+	struct imx_sc_msg_misc_get_soc_id msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	u32 rev;
+	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.send.control = IMX_SC_C_ID;
+	msg.data.send.resource = IMX_SC_R_SYSTEM;
+
+	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
+	if (ret) {
+		dev_err(&imx_scu_soc_pdev->dev,
+			"get soc info failed, ret %d\n", ret);
+		/* return 0 means getting revision failed */
+		return 0;
+	}
+
+	/* format revision value passed from SCU firmware */
+	rev = (msg.data.resp.id >> 5) & 0xf;
+	rev = (((rev >> 2) + 1) << 4) | (rev & 0x3);
+
+	return rev;
+}
+
+static const struct imx_scu_soc_data imx8qxp_soc_data = {
+	.name = "i.MX8QXP",
+	.soc_revision = imx8qxp_soc_revision,

This is still follow a generic soc driver design.
I wonder if this is still needed after separate scu soc driver from imx8m.
If not needed, this driver probably could be simplied a lot.

Using generic soc driver is just to provide different soc data for different SoCs,
NOT sure if we can cover all SCU SoCs using same settings. Different SoCs may
need some different operations or workarounds, like i.MX8QM needs some special
workaround settings....

quoted

+};
+
+static const struct of_device_id imx_scu_soc_match[] = {
+	{ .compatible = "fsl,imx8qxp", .data = &imx8qxp_soc_data, },
+	{ }
+};
+
+#define imx_scu_revision(soc_rev) \
+	soc_rev ? \
+	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &

0xf) : \

quoted

+	"unknown"
+
+static int imx_scu_soc_probe(struct platform_device *pdev) {
+	struct soc_device_attribute *soc_dev_attr;
+	const struct imx_scu_soc_data *data;
+	const struct of_device_id *id;
+	struct soc_device *soc_dev;
+	u32 soc_rev = 0;
+	int ret;
+
+	/* wait i.MX SCU driver ready */
+	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(pdev->dev.of_node,
+				      "model",
+				      &soc_dev_attr->machine);
+	if (ret)
+		return ret;
+
+	id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
+	data = id->data;
+	if (data) {
+		soc_dev_attr->soc_id = data->name;
+		if (data->soc_revision)
+			soc_rev = data->soc_revision();
+	}
+
+	soc_dev_attr->revision = imx_scu_revision(soc_rev);
+	if (!soc_dev_attr->revision)
+		return -ENODEV;
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr->revision);
+		return PTR_ERR(soc_dev);
+	}
+
+	return 0;
+}
+
+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) {
+	const struct of_device_id *id;
+	struct device_node *root;
+	int ret;
+
+	root = of_find_node_by_path("/");
+	id = of_match_node(imx_scu_soc_match, root);

Use of_find_compatible_node instead.
Then you remove two unnecessary local variable.

OK.

BTW, probably a better way is to match "fsl,imx-scu"?

"fsl,imx-scu" already used by scu firmware driver, that will cause confusion
I think, just using soc's compatible should be easier to understand.

quoted

+	if (!id) {
+		of_node_put(root);
+		return -ENODEV;
+	}
+
+	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,

quoted

+						-1,
+						NULL,
+						0);
+	if (IS_ERR(imx_scu_soc_pdev)) {
+		ret = PTR_ERR(imx_scu_soc_pdev);
+		goto unreg_platform_driver;
+	}
+
+	imx_scu_soc_pdev->dev.of_node = root;

Please double check if we really need a global imx_scu_soc_pdev.

Global platform_device pointer can be used in other function's message
output using dev_xxx instead of pr_xxx, and it could be needed later for some
other functions, if no big concern of keeping one global variable, I prefer to keep it.

Thanks,
Anson 

Regards
Dong Aisheng

quoted

+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&imx_scu_soc_driver);
+	return ret;
+}
+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

From: Anson Huang
Sent: Thursday, May 16, 2019 8:01 PM

quoted

quoted

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 i.MX8QXP

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 <redacted>
---
Changes since V2:
	- using device_initcall instead of module_init;
	- check of_match_node in init function and ONLY register platform
driver when matched, this
	  is to avoid unnecessary probe for non SCU based i.MX8 SoCs.
---
 drivers/soc/imx/Kconfig       |   9 +++
 drivers/soc/imx/Makefile      |   1 +
 drivers/soc/imx/soc-imx-scu.c | 173
++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 drivers/soc/imx/soc-imx-scu.c

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index

d80f899..cbc1a41 100644

--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig

@@ -7,4 +7,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 d6b529e0..ddf343d 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile

@@ -1,3 +1,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..243c418

--- /dev/null
+++ b/drivers/soc/imx/soc-imx-scu.c

@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <dt-bindings/firmware/imx/rsrc.h> #include
+<linux/firmware/imx/sci.h> #include <linux/module.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; static struct
+platform_device *imx_scu_soc_pdev;
+
+struct imx_sc_msg_misc_get_soc_id {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct {
+			u32 control;
+			u16 resource;
+		} send;
+		struct {
+			u32 id;
+			u16 reserved;
+		} resp;
+	} data;
+} __packed;
+
+struct imx_scu_soc_data {
+	char *name;
+	u32 (*soc_revision)(void);
+};
+
+static u32 imx8qxp_soc_revision(void) {
+	struct imx_sc_msg_misc_get_soc_id msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	u32 rev;
+	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.send.control = IMX_SC_C_ID;
+	msg.data.send.resource = IMX_SC_R_SYSTEM;
+
+	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
+	if (ret) {
+		dev_err(&imx_scu_soc_pdev->dev,
+			"get soc info failed, ret %d\n", ret);
+		/* return 0 means getting revision failed */
+		return 0;
+	}
+
+	/* format revision value passed from SCU firmware */
+	rev = (msg.data.resp.id >> 5) & 0xf;
+	rev = (((rev >> 2) + 1) << 4) | (rev & 0x3);
+
+	return rev;
+}
+
+static const struct imx_scu_soc_data imx8qxp_soc_data = {
+	.name = "i.MX8QXP",
+	.soc_revision = imx8qxp_soc_revision,

This is still follow a generic soc driver design.
I wonder if this is still needed after separate scu soc driver from imx8m.
If not needed, this driver probably could be simplied a lot.

Using generic soc driver is just to provide different soc data for different SoCs,
NOT sure if we can cover all SCU SoCs using same settings. Different SoCs may
need some different operations or workarounds, like i.MX8QM needs some
special workaround settings....

Mx8qm seems can fully re-use imx8qxp_soc_revision().
And the workaround in local tree seems not need to be put there.
Then the left one is only soc name?

quoted

quoted

+};
+
+static const struct of_device_id imx_scu_soc_match[] = {
+	{ .compatible = "fsl,imx8qxp", .data = &imx8qxp_soc_data, },
+	{ }
+};
+
+#define imx_scu_revision(soc_rev) \
+	soc_rev ? \
+	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &

0xf) : \

quoted

+	"unknown"
+
+static int imx_scu_soc_probe(struct platform_device *pdev) {
+	struct soc_device_attribute *soc_dev_attr;
+	const struct imx_scu_soc_data *data;
+	const struct of_device_id *id;
+	struct soc_device *soc_dev;
+	u32 soc_rev = 0;
+	int ret;
+
+	/* wait i.MX SCU driver ready */
+	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(pdev->dev.of_node,
+				      "model",
+				      &soc_dev_attr->machine);
+	if (ret)
+		return ret;
+
+	id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
+	data = id->data;
+	if (data) {
+		soc_dev_attr->soc_id = data->name;
+		if (data->soc_revision)
+			soc_rev = data->soc_revision();
+	}
+
+	soc_dev_attr->revision = imx_scu_revision(soc_rev);
+	if (!soc_dev_attr->revision)
+		return -ENODEV;
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr->revision);
+		return PTR_ERR(soc_dev);
+	}
+
+	return 0;
+}
+
+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) {
+	const struct of_device_id *id;
+	struct device_node *root;
+	int ret;
+
+	root = of_find_node_by_path("/");
+	id = of_match_node(imx_scu_soc_match, root);

Use of_find_compatible_node instead.
Then you remove two unnecessary local variable.

OK.

quoted

BTW, probably a better way is to match "fsl,imx-scu"?

"fsl,imx-scu" already used by scu firmware driver, that will cause confusion I
think, just using soc's compatible should be easier to understand.

It does not matter because we only use it to check whether we need register
this soc driver. And what I suggest is because this drive actually depends on
"fsl,imx-scu", not soc compatible string.
Another benefit is you do not need another compatible string when you
Add imx8qm support.

quoted

quoted

+	if (!id) {
+		of_node_put(root);
+		return -ENODEV;
+	}
+
+	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,

quoted

+						-1,
+						NULL,
+						0);
+	if (IS_ERR(imx_scu_soc_pdev)) {
+		ret = PTR_ERR(imx_scu_soc_pdev);
+		goto unreg_platform_driver;
+	}
+
+	imx_scu_soc_pdev->dev.of_node = root;

Please double check if we really need a global imx_scu_soc_pdev.

Global platform_device pointer can be used in other function's message output
using dev_xxx instead of pr_xxx, and it could be needed later for some other
functions, if no big concern of keeping one global variable, I prefer to keep it.

I mean If we remove soc_revision() callback, we may not need it.
And imx_scu_soc_match array could also be removed. Then the driver could
Be simplified a lot.

You can check if we can do that.

Regards
Dong Aisheng

Thanks,
Anson

quoted

Regards
Dong Aisheng

quoted

+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&imx_scu_soc_driver);
+	return ret;
+}
+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