| Message ID | 1558586911-29309-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V6,1/2] soc: imx: Add SCU SoC info driver support | expand |
On Thu, May 23, 2019 at 04:53:46AM +0000, 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. This seems buggy... > +static int imx_scu_soc_probe(struct platform_device *pdev) > +{ > + struct soc_device_attribute *soc_dev_attr; > + struct soc_device *soc_dev; > + u32 id, val; > + int ret; > + > + 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(of_root, > + "model", > + &soc_dev_attr->machine); > + if (ret) > + return ret; > + > + id = imx_scu_soc_id(); > + > + /* format soc_id value passed from SCU firmware */ > + val = id & 0x1f; > + soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val) > + : "unknown"; soc_id can either be an allocated string, or it can be a static string of "unknown". > + if (!soc_dev_attr->soc_id) > + return -ENOMEM; > + > + /* format revision value passed from SCU firmware */ > + val = (id >> 5) & 0xf; > + val = (((val >> 2) + 1) << 4) | (val & 0x3); > + soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL, > + "%d.%d", > + (val >> 4) & 0xf, > + val & 0xf) : "unknown"; revision here is the same. > + if (!soc_dev_attr->revision) { > + ret = -ENOMEM; > + goto free_soc_id; > + } > + > + soc_dev = soc_device_register(soc_dev_attr); > + if (IS_ERR(soc_dev)) { > + ret = PTR_ERR(soc_dev); > + goto free_revision; > + } > + > + return 0; > + > +free_revision: > + kfree(soc_dev_attr->revision); > +free_soc_id: > + kfree(soc_dev_attr->soc_id); Yet here you blindly kfree(), which means you could be doing in effect kfree("unknown") - which will likely result in a kernel oops. Either always make revision/soc_id an allocated string, or use some static storage for the strings (they're only small, static storage is likely to be much more efficient in this case, and there can only be one of these in any case.) > + return ret; > +} > + > +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) > +{ > + struct platform_device *imx_scu_soc_pdev; > + struct device_node *np; > + int ret; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu"); > + if (!np) > + return -ENODEV; > + > + of_node_put(np); > + > + 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)) > + platform_driver_unregister(&imx_scu_soc_driver); > + > + return PTR_ERR_OR_ZERO(imx_scu_soc_pdev); > +} > +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 >
Hi, Russell > -----Original Message----- > From: Russell King - ARM Linux admin [mailto:linux@armlinux.org.uk] > Sent: Thursday, May 23, 2019 5:10 PM > To: Anson Huang <anson.huang@nxp.com> > Cc: catalin.marinas@arm.com; will.deacon@arm.com; > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; maxime.ripard@bootlin.com; olof@lixom.net; > agross@kernel.org; jagan@amarulasolutions.com; > bjorn.andersson@linaro.org; Leonard Crestez <leonard.crestez@nxp.com>; > dinguyen@kernel.org; enric.balletbo@collabora.com; Aisheng Dong > <aisheng.dong@nxp.com>; Abel Vesa <abel.vesa@nxp.com>; > l.stach@pengutronix.de; tglx@linutronix.de; robh@kernel.org; > gregkh@linuxfoundation.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support > > On Thu, May 23, 2019 at 04:53:46AM +0000, 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. > > This seems buggy... We have to use platform driver model, but there is no such soc device in DT file, so we created the platform device in this driver directly. > > > +static int imx_scu_soc_probe(struct platform_device *pdev) { > > + struct soc_device_attribute *soc_dev_attr; > > + struct soc_device *soc_dev; > > + u32 id, val; > > + int ret; > > + > > + 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(of_root, > > + "model", > > + &soc_dev_attr->machine); > > + if (ret) > > + return ret; > > + > > + id = imx_scu_soc_id(); > > + > > + /* format soc_id value passed from SCU firmware */ > > + val = id & 0x1f; > > + soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val) > > + : "unknown"; > > soc_id can either be an allocated string, or it can be a static string of > "unknown". > > > + if (!soc_dev_attr->soc_id) > > + return -ENOMEM; > > + > > + /* format revision value passed from SCU firmware */ > > + val = (id >> 5) & 0xf; > > + val = (((val >> 2) + 1) << 4) | (val & 0x3); > > + soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL, > > + "%d.%d", > > + (val >> 4) & 0xf, > > + val & 0xf) : "unknown"; > > revision here is the same. > > > + if (!soc_dev_attr->revision) { > > + ret = -ENOMEM; > > + goto free_soc_id; > > + } > > + > > + soc_dev = soc_device_register(soc_dev_attr); > > + if (IS_ERR(soc_dev)) { > > + ret = PTR_ERR(soc_dev); > > + goto free_revision; > > + } > > + > > + return 0; > > + > > +free_revision: > > + kfree(soc_dev_attr->revision); > > +free_soc_id: > > + kfree(soc_dev_attr->soc_id); > > Yet here you blindly kfree(), which means you could be doing in effect > kfree("unknown") - which will likely result in a kernel oops. > > Either always make revision/soc_id an allocated string, or use some static > storage for the strings (they're only small, static storage is likely to be much > more efficient in this case, and there can only be one of these in any case.) You are right, actually the "unknown" message is NOT helpful at all, so in next version, I will just check the return value from SCU firmware, if it fail, just skip this driver probe, If it success, it will just print out the soc_id and revision passing from SCU firmware, so no need to have "unknown" string here. +++ b/drivers/soc/imx/soc-imx-scu.c @@ -27,7 +27,7 @@ struct imx_sc_msg_misc_get_soc_id { } data; } __packed; -static u32 imx_scu_soc_id(void) +static int imx_scu_soc_id(void) { struct imx_sc_msg_misc_get_soc_id msg; struct imx_sc_rpc_msg *hdr = &msg.hdr; @@ -44,8 +44,7 @@ static u32 imx_scu_soc_id(void) ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true); if (ret) { pr_err("%s: get soc info failed, ret %d\n", __func__, ret); - /* return 0 means getting revision failed */ - return 0; + return ret; } return msg.data.resp.id; @@ -55,7 +54,8 @@ static int imx_scu_soc_probe(struct platform_device *pdev) { struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; - u32 id, val; + u32 val; + int id; int ret; ret = imx_scu_get_handle(&soc_ipc_handle); @@ -76,21 +76,22 @@ static int imx_scu_soc_probe(struct platform_device *pdev) return ret; id = imx_scu_soc_id(); + if (id < 0) + return -EINVAL; /* format soc_id value passed from SCU firmware */ val = id & 0x1f; - soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val) - : "unknown"; + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", val); if (!soc_dev_attr->soc_id) return -ENOMEM; /* format revision value passed from SCU firmware */ val = (id >> 5) & 0xf; val = (((val >> 2) + 1) << 4) | (val & 0x3); - soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL, - "%d.%d", - (val >> 4) & 0xf, - val & 0xf) : "unknown"; + soc_dev_attr->revision = kasprintf(GFP_KERNEL, + "%d.%d", + (val >> 4) & 0xf, + val & 0xf); if (!soc_dev_attr->revision) { Thanks, Anson. > > > + return ret; > > +} > > + > > +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) { > > + struct platform_device *imx_scu_soc_pdev; > > + struct device_node *np; > > + int ret; > > + > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu"); > > + if (!np) > > + return -ENODEV; > > + > > + of_node_put(np); > > + > > + 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)) > > + platform_driver_unregister(&imx_scu_soc_driver); > > + > > + return PTR_ERR_OR_ZERO(imx_scu_soc_pdev); > > +} > > +device_initcall(imx_scu_soc_init); > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists > > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm- > kernel&data=02%7C0 > > > 1%7Canson.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C > 686ea1d > > > 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&sdat > a=mXdow > > > fLshJ%2BQlyLJ%2BbyYwuDUXh3CwOBAiZ%2BvVED%2FRh4%3D&reserve > d=0 > > > > -- > RMK's Patch system: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > armlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Canso > n.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C686ea1d3 > bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&sdata > =rsrH%2F9pqdLZUJ0E%2BWUa2Kf71rtOcNDyjJ81R8Ex6yQs%3D&reserve > d=0 > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down > 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index ade1b46..8aaebf1 100644 --- a/drivers/soc/imx/Kconfig +++ b/drivers/soc/imx/Kconfig @@ -8,4 +8,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 caa8653..cf9ca42 100644 --- a/drivers/soc/imx/Makefile +++ b/drivers/soc/imx/Makefile @@ -2,3 +2,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..258c987 --- /dev/null +++ b/drivers/soc/imx/soc-imx-scu.c @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2019 NXP. + */ + +#include <dt-bindings/firmware/imx/rsrc.h> +#include <linux/firmware/imx/sci.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; + +struct imx_sc_msg_misc_get_soc_id { + struct imx_sc_rpc_msg hdr; + union { + struct { + u32 control; + u16 resource; + } __packed req; + struct { + u32 id; + } __packed resp; + } data; +} __packed; + +static u32 imx_scu_soc_id(void) +{ + struct imx_sc_msg_misc_get_soc_id msg; + struct imx_sc_rpc_msg *hdr = &msg.hdr; + 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.req.control = IMX_SC_C_ID; + msg.data.req.resource = IMX_SC_R_SYSTEM; + + ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true); + if (ret) { + pr_err("%s: get soc info failed, ret %d\n", __func__, ret); + /* return 0 means getting revision failed */ + return 0; + } + + return msg.data.resp.id; +} + +static int imx_scu_soc_probe(struct platform_device *pdev) +{ + struct soc_device_attribute *soc_dev_attr; + struct soc_device *soc_dev; + u32 id, val; + int ret; + + 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(of_root, + "model", + &soc_dev_attr->machine); + if (ret) + return ret; + + id = imx_scu_soc_id(); + + /* format soc_id value passed from SCU firmware */ + val = id & 0x1f; + soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val) + : "unknown"; + if (!soc_dev_attr->soc_id) + return -ENOMEM; + + /* format revision value passed from SCU firmware */ + val = (id >> 5) & 0xf; + val = (((val >> 2) + 1) << 4) | (val & 0x3); + soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL, + "%d.%d", + (val >> 4) & 0xf, + val & 0xf) : "unknown"; + if (!soc_dev_attr->revision) { + ret = -ENOMEM; + goto free_soc_id; + } + + soc_dev = soc_device_register(soc_dev_attr); + if (IS_ERR(soc_dev)) { + ret = PTR_ERR(soc_dev); + goto free_revision; + } + + return 0; + +free_revision: + kfree(soc_dev_attr->revision); +free_soc_id: + kfree(soc_dev_attr->soc_id); + return ret; +} + +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) +{ + struct platform_device *imx_scu_soc_pdev; + struct device_node *np; + int ret; + + np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu"); + if (!np) + return -ENODEV; + + of_node_put(np); + + 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)) + platform_driver_unregister(&imx_scu_soc_driver); + + return PTR_ERR_OR_ZERO(imx_scu_soc_pdev); +} +device_initcall(imx_scu_soc_init);