Message-ID: <DB3PR0402MB3916EAB4C1195F84BEA1CA80F50C0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Fri, 10 May 2019 11:15:01 +0000
From:   Anson Huang <anson.huang@....com>
To:     Aisheng Dong <aisheng.dong@....com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "heiko@...ech.de" <heiko@...ech.de>,
        "horms+renesas@...ge.net.au" <horms+renesas@...ge.net.au>,
        "olof@...om.net" <olof@...om.net>,
        Andy Gross <andy.gross@...aro.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "jagan@...rulasolutions.com" <jagan@...rulasolutions.com>,
        "enric.balletbo@...labora.com" <enric.balletbo@...labora.com>,
        "stefan.wahren@...e.com" <stefan.wahren@...e.com>,
        "ezequiel@...labora.com" <ezequiel@...labora.com>,
        "marc.w.gonzalez@...e.fr" <marc.w.gonzalez@...e.fr>,
        "robh@...nel.org" <robh@...nel.org>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        Abel Vesa <abel.vesa@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc driver
 support

Hi, Aisheng
	Thanks for comments, I plan to resend this patch to make it align with current drivers/soc/imx/soc-imx8.c implementation about the soc id/revision generation, the ONLY difference would be the system controller SoC driver needs to use platform driver model as it needs to use defer probe to make sure SCU driver is ready, and need to use SCU API for getting soc id, will resend a patch soon, sorry for wasting your time on this patch.

Anson.

> -----Original Message-----
> From: Aisheng Dong
> Sent: Friday, May 10, 2019 5:17 PM
> To: Anson Huang <anson.huang@....com>; catalin.marinas@....com;
> will.deacon@....com; shawnguo@...nel.org; s.hauer@...gutronix.de;
> kernel@...gutronix.de; festevam@...il.com; heiko@...ech.de;
> horms+renesas@...ge.net.au; olof@...om.net; Andy Gross
> <andy.gross@...aro.org>; bjorn.andersson@...aro.org;
> jagan@...rulasolutions.com; enric.balletbo@...labora.com;
> stefan.wahren@...e.com; ezequiel@...labora.com;
> marc.w.gonzalez@...e.fr; robh@...nel.org; l.stach@...gutronix.de; Abel
> Vesa <abel.vesa@....com>; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org
> Cc: dl-linux-imx <linux-imx@....com>
> Subject: RE: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc driver
> support
> 
> > From: Anson Huang
> > Sent: Thursday, April 11, 2019 2:49 PM
> >
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock and
> fuse etc..
> >
> > This patch adds i.MX system controller soc driver support, Linux
> > kernel has to communicate with system controller via MU (message unit)
> > IPC to get soc revision, uid etc..
> >
> > 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
> >
> > i.mx8qxp-mek# cat /sys/devices/soc0/soc_uid
> > 7B64280B57AC1898
> >
> > Signed-off-by: Anson Huang <Anson.Huang@....com>
> > ---
> >  drivers/soc/imx/Kconfig      |   7 ++
> >  drivers/soc/imx/Makefile     |   1 +
> >  drivers/soc/imx/soc-imx-sc.c | 220
> > +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 228 insertions(+)
> >  create mode 100644 drivers/soc/imx/soc-imx-sc.c
> >
> > diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index
> > d80f899..c902b89 100644
> > --- a/drivers/soc/imx/Kconfig
> > +++ b/drivers/soc/imx/Kconfig
> > @@ -7,4 +7,11 @@ config IMX_GPCV2_PM_DOMAINS
> >  	select PM_GENERIC_DOMAINS
> >  	default y if SOC_IMX7D
> >
> > +config IMX_SC_SOC
> > +	depends on IMX_SCU || COMPILE_TEST
> 
> COMPILE_TEST may not work due to dependency
> 
> > +	tristate "i.MX System Controller SoC support"
> 
> Can it build as module?
> I did not see soc_device_register() is exported.
> 
> > +	help
> > +	   If you say yes here you get support for the i.MX System
> > +	   Controller SoC module.
> > +
> >  endmenu
> > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > 506a6f3..d00606d 100644
> > --- a/drivers/soc/imx/Makefile
> > +++ b/drivers/soc/imx/Makefile
> > @@ -1,2 +1,3 @@
> >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > +obj-$(CONFIG_IMX_SC_SOC) += soc-imx-sc.o
> > diff --git a/drivers/soc/imx/soc-imx-sc.c
> > b/drivers/soc/imx/soc-imx-sc.c new file mode 100644 index
> > 0000000..029d754
> > --- /dev/null
> > +++ b/drivers/soc/imx/soc-imx-sc.c
> > @@ -0,0 +1,220 @@
> > +// 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>
> > +
> > +#include <soc/imx/revision.h>
> > +
> > +#define IMX_SC_SOC_DRIVER_NAME	"imx-sc-soc"
> > +
> > +#define SOC_REV_MAJOR_OFFSET	0x4
> > +#define SOC_REV_MAJOR_MASK	0xf
> > +#define SOC_REV_MINOR_OFFSET	0x4
> > +#define SOC_REV_MINOR_MASK	0xf
> > +
> > +#define get_soc_rev_major(rev) ((rev >> SOC_REV_MAJOR_OFFSET) &
> > +SOC_REV_MAJOR_MASK) #define get_soc_rev_minor(rev) ((rev >>
> > +SOC_REV_MINOR_OFFSET) & SOC_REV_MINOR_MASK)
> > +
> > +static u32 imx_sc_soc_rev = IMX_CHIP_REVISION_UNKNOWN; static u64
> > +imx_sc_soc_uid;
> > +
> > +static struct imx_sc_ipc *soc_ipc_handle; static struct
> > +platform_device *imx_sc_soc_pdev;
> > +
> > +struct imx_sc_msg_misc_get_soc_id {
> > +	struct imx_sc_rpc_msg hdr;
> > +	union {
> > +		struct {
> > +			u32 control;
> > +			u16 resource;
> > +		} __packed send;
> > +		struct {
> > +			u32 id;
> > +			u16 reserved;
> > +		} __packed resp;
> > +	} data;
> > +};
> 
> By learned more, I think probably a more safe reference is to have one more
> __packed outside. Then we can unified in this way.
> 
> > +
> > +struct imx_sc_msg_misc_get_soc_uid {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u32 id_l;
> > +	u32 id_h;
> > +};
> > +
> > +static inline void imx_sc_set_soc_revision(u32 rev) {
> > +	imx_sc_soc_rev = rev;
> > +}
> > +
> > +unsigned int imx_get_soc_revision(void) {
> > +	return imx_sc_soc_rev;
> > +}
> > +EXPORT_SYMBOL(imx_get_soc_revision);
> > +
> > +static u32 imx_init_revision_from_scu(void) {
> > +	struct imx_sc_msg_misc_get_soc_id msg;
> > +	struct imx_sc_msg_misc_get_soc_uid msg1;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	struct imx_sc_rpc_msg *hdr1 = &msg1.hdr;
> > +	u32 id, 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) {
> > +		pr_err("misc get control failed, ret %d\n", ret);
> 
> Pls improve the message
> 
> > +		return ret;
> > +	}
> > +
> > +	id = msg.data.resp.id;
> > +
> > +	rev = (id >> 5) & 0xf;
> > +	rev = (((rev >> 2) + 1) << 4) | (rev & 0x3);
> > +
> > +	imx_sc_set_soc_revision(rev);
> > +
> > +	hdr1->ver = IMX_SC_RPC_VERSION;
> > +	hdr1->svc = IMX_SC_RPC_SVC_MISC;
> > +	hdr1->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
> > +	hdr1->size = 1;
> 
> Can't we reuse the first one?
> 
> > +
> > +	/* the return value of SCU FW is in correct, can NOT check the ret */
> > +	ret = imx_scu_call_rpc(soc_ipc_handle, &msg1, true);
> 
> If can't check ret, then do not assign?
> 
> But how do we make sure the function call is successfully?
> How about check other returns? E.g. -ETIMEOUT?
> 
> > +
> > +	imx_sc_soc_uid = msg1.id_h;
> > +	imx_sc_soc_uid <<= 32;
> > +	imx_sc_soc_uid |= msg1.id_l;
> > +
> > +	return rev;
> > +}
> > +
> > +static ssize_t imx_sc_get_soc_uid(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  char *buf)
> > +{
> > +	return sprintf(buf, "%016llX\n", imx_sc_soc_uid); }
> > +
> > +static struct device_attribute imx_sc_uid =
> > +	__ATTR(soc_uid, 0444, imx_sc_get_soc_uid, NULL);
> > +
> > +static int imx_sc_soc_probe(struct platform_device *pdev) {
> > +	struct soc_device_attribute *soc_dev_attr;
> > +	u32 rev = IMX_CHIP_REVISION_UNKNOWN;
> > +	struct soc_device *soc_dev;
> > +	u32 soc_rev;
> > +	int ret;
> > +
> > +	ret = imx_scu_get_handle(&soc_ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > +	if (!soc_dev_attr)
> > +		return -ENOMEM;
> > +
> > +	soc_dev_attr->family = "Freescale i.MX";
> > +
> > +	if (of_machine_is_compatible("fsl,imx8qxp"))
> > +		soc_dev_attr->soc_id = "i.MX8QXP";
> > +	else
> > +		soc_dev_attr->soc_id = "unknown";
> 
> Why not just return directly? Then we can remove the unknow chip support.
> Or we must have to support an unkown chip?
> 
> > +
> > +	rev = imx_init_revision_from_scu();
> > +	if (rev == IMX_CHIP_REVISION_UNKNOWN)
> > +		dev_info(&pdev->dev, "CPU identified as %s, unknown
> revision\n",
> > +			 soc_dev_attr->soc_id);
> > +	else
> > +		dev_info(&pdev->dev, "CPU identified as %s, silicon
> rev %d.%d\n",
> > +			 soc_dev_attr->soc_id,
> > +			 get_soc_rev_major(rev),
> > +			 get_soc_rev_minor(rev));
> > +
> > +	soc_rev = imx_get_soc_revision();
> > +	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d.%d",
> > +					   get_soc_rev_major(rev),
> > +					   get_soc_rev_minor(rev));
> > +	if (!soc_dev_attr->revision) {
> > +		ret = -ENOMEM;
> > +		goto free_soc;
> > +	}
> > +
> > +	of_property_read_string(of_root, "model", &soc_dev_attr-
> >machine);
> > +
> > +	soc_dev = soc_device_register(soc_dev_attr);
> > +	if (IS_ERR(soc_dev)) {
> > +		ret = PTR_ERR(soc_dev);
> > +		goto free_rev;
> > +	}
> > +
> > +	ret = device_create_file(soc_device_to_device(soc_dev),
> &imx_sc_uid);
> > +	if (ret)
> > +		dev_err(&pdev->dev, "could not register sysfs entry\n");
> 
> Improve the message
> 
> > +
> > +	return ret;
> > +
> > +free_rev:
> > +	kfree(soc_dev_attr->revision);
> > +free_soc:
> > +	kfree(soc_dev_attr);
> 
> If using platform device model, we may use devm_x API as well.
> 
> However, I'm a bit wondering whether it's really necessary to model It as
> platform device?
> 
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver imx_sc_soc_driver = {
> > +	.driver = {
> > +		.name = IMX_SC_SOC_DRIVER_NAME,
> > +	},
> > +	.probe = imx_sc_soc_probe,
> > +};
> > +
> > +static int __init imx_sc_soc_init(void) {
> > +	int ret;
> > +
> > +	ret = platform_driver_register(&imx_sc_soc_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	imx_sc_soc_pdev =
> > +
> 	platform_device_register_simple(IMX_SC_SOC_DRIVER_NAME,
> > +						-1,
> > +						NULL,
> > +						0);
> 
> Is it really necessary?
> 
> Regards
> Dong Aisheng
> 
> > +	if (IS_ERR(imx_sc_soc_pdev)) {
> > +		ret = PTR_ERR(imx_sc_soc_pdev);
> > +		goto unreg_platform_driver;
> > +	}
> > +
> > +	return 0;
> > +
> > +unreg_platform_driver:
> > +	platform_driver_unregister(&imx_sc_soc_driver);
> > +	return ret;
> > +}
> > +
> > +static void __exit imx_sc_soc_exit(void) {
> > +	platform_device_unregister(imx_sc_soc_pdev);
> > +	platform_driver_unregister(&imx_sc_soc_driver);
> > +}
> > +
> > +module_init(imx_sc_soc_init);
> > +module_exit(imx_sc_soc_exit);
> > --
> > 2.7.4

Powered by blists - more mailing lists