> 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.hu...@nxp.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
