linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
@ 2019-05-16  3:24 Anson Huang
  2019-05-16  3:24 ` [PATCH V3 2/2] arm64: defconfig: Add i.MX SCU SoC info driver Anson Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Anson Huang @ 2019-05-16  3:24 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, shawnguo, s.hauer, kernel,
	festevam, agross, maxime.ripard, olof, horms+renesas, jagan,
	bjorn.andersson, Leonard Crestez, marc.w.gonzalez, dinguyen,
	enric.balletbo, l.stach, Aisheng Dong, Abel Vesa, robh,
	linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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 <Anson.Huang@nxp.com>
---
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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH V3 2/2] arm64: defconfig: Add i.MX SCU SoC info driver
  2019-05-16  3:24 [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Anson Huang
@ 2019-05-16  3:24 ` Anson Huang
  2019-05-16 10:06 ` [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Leonard Crestez
  2019-05-16 11:12 ` Aisheng Dong
  2 siblings, 0 replies; 7+ messages in thread
From: Anson Huang @ 2019-05-16  3:24 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, shawnguo, s.hauer, kernel,
	festevam, agross, maxime.ripard, olof, horms+renesas, jagan,
	bjorn.andersson, Leonard Crestez, marc.w.gonzalez, dinguyen,
	enric.balletbo, l.stach, Aisheng Dong, Abel Vesa, robh,
	linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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>
---
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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
  2019-05-16  3:24 [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Anson Huang
  2019-05-16  3:24 ` [PATCH V3 2/2] arm64: defconfig: Add i.MX SCU SoC info driver Anson Huang
@ 2019-05-16 10:06 ` Leonard Crestez
  2019-05-16 11:09   ` Anson Huang
  2019-05-16 11:12 ` Aisheng Dong
  2 siblings, 1 reply; 7+ messages in thread
From: Leonard Crestez @ 2019-05-16 10:06 UTC (permalink / raw)
  To: Anson Huang, shawnguo
  Cc: catalin.marinas, will.deacon, s.hauer, kernel, festevam, agross,
	maxime.ripard, olof, horms+renesas, jagan, bjorn.andersson,
	marc.w.gonzalez, dinguyen, enric.balletbo, l.stach, Aisheng Dong,
	Abel Vesa, robh, linux-arm-kernel, linux-kernel, dl-linux-imx

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.

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

> +	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.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
  2019-05-16 10:06 ` [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Leonard Crestez
@ 2019-05-16 11:09   ` Anson Huang
  0 siblings, 0 replies; 7+ messages in thread
From: Anson Huang @ 2019-05-16 11:09 UTC (permalink / raw)
  To: Leonard Crestez, shawnguo
  Cc: catalin.marinas, will.deacon, s.hauer, kernel, festevam, agross,
	maxime.ripard, olof, horms+renesas, jagan, bjorn.andersson,
	marc.w.gonzalez, dinguyen, enric.balletbo, l.stach, Aisheng Dong,
	Abel Vesa, robh, linux-arm-kernel, linux-kernel, dl-linux-imx

Hi, Leonard

> -----Original Message-----
> From: Leonard Crestez
> Sent: Thursday, May 16, 2019 6:07 PM
> To: Anson Huang <anson.huang@nxp.com>; 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 <aisheng.dong@nxp.com>; Abel Vesa
> <abel.vesa@nxp.com>; robh@kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
> 
> 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.
> 
> > +#define imx_scu_revision(soc_rev) \
> > +	soc_rev ? \
> > +	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &
> 0xf) : \
> > +	"unknown"
> 
> > +	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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
  2019-05-16  3:24 [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Anson Huang
  2019-05-16  3:24 ` [PATCH V3 2/2] arm64: defconfig: Add i.MX SCU SoC info driver Anson Huang
  2019-05-16 10:06 ` [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Leonard Crestez
@ 2019-05-16 11:12 ` Aisheng Dong
  2019-05-16 12:01   ` Anson Huang
  2 siblings, 1 reply; 7+ messages in thread
From: Aisheng Dong @ 2019-05-16 11:12 UTC (permalink / raw)
  To: Anson Huang, catalin.marinas, will.deacon, shawnguo, s.hauer,
	kernel, festevam, agross, maxime.ripard, olof, horms+renesas,
	jagan, bjorn.andersson, Leonard Crestez, marc.w.gonzalez,
	dinguyen, enric.balletbo, l.stach, Abel Vesa, robh,
	linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

> 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 <Anson.Huang@nxp.com>
> ---
> 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.

> +};
> +
> +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);

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

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

> +	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;

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
  2019-05-16 11:12 ` Aisheng Dong
@ 2019-05-16 12:01   ` Anson Huang
  2019-05-16 14:09     ` Aisheng Dong
  0 siblings, 1 reply; 7+ messages in thread
From: Anson Huang @ 2019-05-16 12:01 UTC (permalink / raw)
  To: Aisheng Dong, catalin.marinas, will.deacon, shawnguo, s.hauer,
	kernel, festevam, agross, maxime.ripard, olof, horms+renesas,
	jagan, bjorn.andersson, Leonard Crestez, marc.w.gonzalez,
	dinguyen, enric.balletbo, l.stach, Abel Vesa, robh,
	linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx



> -----Original Message-----
> From: Aisheng Dong
> Sent: Thursday, May 16, 2019 7:13 PM
> To: Anson Huang <anson.huang@nxp.com>; 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
> <leonard.crestez@nxp.com>; marc.w.gonzalez@free.fr;
> dinguyen@kernel.org; enric.balletbo@collabora.com;
> l.stach@pengutronix.de; Abel Vesa <abel.vesa@nxp.com>; robh@kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
> 
> > 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 <Anson.Huang@nxp.com>
> > ---
> > 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....

> 
> > +};
> > +
> > +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);
> 
> 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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
  2019-05-16 12:01   ` Anson Huang
@ 2019-05-16 14:09     ` Aisheng Dong
  0 siblings, 0 replies; 7+ messages in thread
From: Aisheng Dong @ 2019-05-16 14:09 UTC (permalink / raw)
  To: Anson Huang, catalin.marinas, will.deacon, shawnguo, s.hauer,
	kernel, festevam, agross, maxime.ripard, olof, horms+renesas,
	jagan, bjorn.andersson, Leonard Crestez, marc.w.gonzalez,
	dinguyen, enric.balletbo, l.stach, Abel Vesa, robh,
	linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

> From: Anson Huang
> Sent: Thursday, May 16, 2019 8:01 PM
> > >
> > > 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 <Anson.Huang@nxp.com>
> > > ---
> > > 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?

> >
> > > +};
> > > +
> > > +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);
> >
> > 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.
> 

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.

> >
> > > +	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;
> >
> > 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
> 
> >
> > 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-05-16 14:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16  3:24 [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Anson Huang
2019-05-16  3:24 ` [PATCH V3 2/2] arm64: defconfig: Add i.MX SCU SoC info driver Anson Huang
2019-05-16 10:06 ` [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support Leonard Crestez
2019-05-16 11:09   ` Anson Huang
2019-05-16 11:12 ` Aisheng Dong
2019-05-16 12:01   ` Anson Huang
2019-05-16 14:09     ` Aisheng Dong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).