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

[Anson Huang]

> -----Original Message-----
> From: Aisheng Dong
> Sent: Thursday, May 16, 2019 7:13 PM
> To: Anson Huang <anson.huang@xxxxxxx>; catalin.marinas@xxxxxxx;
> will.deacon@xxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; agross@xxxxxxxxxx;
> maxime.ripard@xxxxxxxxxxx; olof@xxxxxxxxx; horms+renesas@xxxxxxxxxxxx;
> jagan@xxxxxxxxxxxxxxxxxxxx; bjorn.andersson@xxxxxxxxxx; Leonard Crestez
> <leonard.crestez@xxxxxxx>; marc.w.gonzalez@xxxxxxx;
> dinguyen@xxxxxxxxxx; enric.balletbo@xxxxxxxxxxxxx;
> l.stach@xxxxxxxxxxxxxx; Abel Vesa <abel.vesa@xxxxxxx>; robh@xxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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