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

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