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 <[email protected]>
---
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
+ tristate "i.MX System Controller SoC support"
+ 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;
+};
+
+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);
+ 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;
+
+ /* the return value of SCU FW is in correct, can NOT check the ret */
+ ret = imx_scu_call_rpc(soc_ipc_handle, &msg1, true);
+
+ 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";
+
+ 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");
+
+ return ret;
+
+free_rev:
+ kfree(soc_dev_attr->revision);
+free_soc:
+ kfree(soc_dev_attr);
+ 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);
+ 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
This patch enables CONFIG_IMX_SC_SOC as module.
Signed-off-by: Anson Huang <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d4c404a..b61cc53 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -683,6 +683,7 @@ CONFIG_RPMSG_QCOM_GLINK_SMEM=m
CONFIG_RPMSG_QCOM_SMD=y
CONFIG_RASPBERRYPI_POWER=y
CONFIG_IMX_GPCV2_PM_DOMAINS=y
+CONFIG_IMX_SC_SOC=m
CONFIG_QCOM_COMMAND_DB=y
CONFIG_QCOM_GENI_SE=y
CONFIG_QCOM_GLINK_SSR=m
--
2.7.4
On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> 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 <[email protected]>
> ---
> 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
Rather than creating a new driver, please take a look at Abel's generic
i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
Shawn
On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
> On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> > 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 <[email protected]>
> > ---
> > 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
>
> Rather than creating a new driver, please take a look at Abel's generic
> i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
Forgot to give pointer to Abel's driver.
https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/drivers&id=a7e26f356ca12906a164d83c9e9f8527ee7da022
Shawn
Hi, Shawn
Best Regards!
Anson Huang
> -----Original Message-----
> From: Shawn Guo [mailto:[email protected]]
> Sent: Sunday, April 21, 2019 3:42 PM
> To: Anson Huang <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Abel Vesa
> <[email protected]>; [email protected]; Andy Gross
> <[email protected]>; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc driver
> support
>
> On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
> > On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> > > 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 <[email protected]>
> > > ---
> > > 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
> >
> > Rather than creating a new driver, please take a look at Abel's
> > generic
> > i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
>
> Forgot to give pointer to Abel's driver.
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fshawnguo%2Flinux.git%
> 2Fcommit%2F%3Fh%3Dimx%2Fdrivers%26id%3Da7e26f356ca12906a164d83c
> 9e9f8527ee7da022&data=02%7C01%7Canson.huang%40nxp.com%7C9
> e2705d7449b4c2e23ed08d6c62ce0bb%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636914293400307709&sdata=6ySEs%2B4SE8bvcBCkfoi
> VBafseAYthTED9%2F5qcf25xds%3D&reserved=0
>
Got it, I didn't notice that this patch bas been accepted, I will redo the patch based on it,
thanks.
Anson.
> Shawn
Hi, Shawn
Best Regards!
Anson Huang
> -----Original Message-----
> From: Anson Huang
> Sent: Monday, April 22, 2019 8:52 AM
> To: Shawn Guo <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Abel Vesa
> <[email protected]>; [email protected]; Andy Gross
> <[email protected]>; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: RE: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc driver
> support
>
> Hi, Shawn
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Shawn Guo [mailto:[email protected]]
> > Sent: Sunday, April 21, 2019 3:42 PM
> > To: Anson Huang <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Abel Vesa
> > <[email protected]>; [email protected]; Andy Gross
> > <[email protected]>; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>;
> > [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc
> > driver support
> >
> > On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
> > > On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> > > > 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 <[email protected]>
> > > > ---
> > > > 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
> > >
> > > Rather than creating a new driver, please take a look at Abel's
> > > generic
> > > i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
> >
> > Forgot to give pointer to Abel's driver.
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fshawnguo%2Flinux.git%
> >
> 2Fcommit%2F%3Fh%3Dimx%2Fdrivers%26id%3Da7e26f356ca12906a164d83c
> >
> 9e9f8527ee7da022&data=02%7C01%7Canson.huang%40nxp.com%7C9
> >
> e2705d7449b4c2e23ed08d6c62ce0bb%7C686ea1d3bc2b4c6fa92cd99c5c3016
> >
> 35%7C0%7C0%7C636914293400307709&sdata=6ySEs%2B4SE8bvcBCkfoi
> > VBafseAYthTED9%2F5qcf25xds%3D&reserved=0
> >
>
> Got it, I didn't notice that this patch bas been accepted, I will redo the patch
> based on it, thanks.
I have sent the new patch set to support i.MX8QXP SoC revision based on generic i.MX8
SoC driver, however, the Kconfig modification is NOT good, it may break i.MX8MQ if IMX_SCU
is NOT enabled, although we can add some warp function for SCU firmware API call to fix it,
but after further thought and discussion with Dong Aisheng, I think we may need to roll back to
use this patch series to create a new SoC driver dedicated for i.MX8 SoCs
with system controller inside, such as i.MX8QXP, i.MX8QM etc., the reason are as below:
For i.MX8MQ/i.MX8MM:
1. SoC driver does NOT depends on i.MX SCU firmware, so no need to use platform driver
probe model, just device_init phase call is good enough;
2. The SoC driver no need to depends on IMX_SCU, so it can be always built in, no need to
check IMX_SCU config;
3. The fuse check for CPU speed grading, HDCP status, NoC settings etc. could be added to this driver,
but they are ONLY for i.MX8MQ/i.MX8MM etc..
For i.MX8QXP/i.MX8QM:
1. SoC driver MUST depends on IMX_SCU;
2. MUST use platform model to support defer probe;
3. No fuse check for CPU speed grading.
So, I guess the reused code for i.MX8MQ and i.MX8QXP is ONLY those part of creating SoC id device node (less than
30% I think), all other functions are implemented in total different ways, that is why I created the imx_sc_soc driver
in this patch series, so do you think we can add new SoC driver for i.MX8 SoC with SCU inside? Putting 2 different architecture
SoCs' driver into 1 file looks like NOT making enough sense.
Anson.
>
> Anson.
>
> > Shawn
On 4/22/2019 9:46 AM, Anson Huang wrote:
>> -----Original Message-----
>> From: Anson Huang
>>> From: Shawn Guo [mailto:[email protected]]
>>> On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
>>>> On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
>>>>> 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:
>>>>>
>>>>> 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
>>>>
>>>> Rather than creating a new driver, please take a look at Abel's
>>>> generic
>>>> i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
>>
>> Got it, I didn't notice that this patch bas been accepted, I will redo the patch
>> based on it, thanks.
>
> I have sent the new patch set to support i.MX8QXP SoC revision based on generic i.MX8
> SoC driver, however, the Kconfig modification is NOT good, it may break i.MX8MQ if IMX_SCU
> is NOT enabled, although we can add some warp function for SCU firmware API call to fix it,
> but after further thought and discussion with Dong Aisheng, I think we may need to roll back to
> use this patch series to create a new SoC driver dedicated for i.MX8 SoCs
> with system controller inside, such as i.MX8QXP, i.MX8QM etc., the reason are as below:
>
> For i.MX8MQ/i.MX8MM:
> 1. SoC driver does NOT depends on i.MX SCU firmware, so no need to use platform driver
> probe model, just device_init phase call is good enough;
> 2. The SoC driver no need to depends on IMX_SCU, so it can be always built in, no need to
> check IMX_SCU config;
> 3. The fuse check for CPU speed grading, HDCP status, NoC settings etc. could be added to this driver,
> but they are ONLY for i.MX8MQ/i.MX8MM etc..
> For i.MX8QXP/i.MX8QM:
> 1. SoC driver MUST depends on IMX_SCU;
> 2. MUST use platform model to support defer probe;
> 3. No fuse check for CPU speed grading.
>
> So, I guess the reused code for i.MX8MQ and i.MX8QXP is ONLY those part of creating SoC id device node (less than
> 30% I think), all other functions are implemented in total different ways, that is why I created the imx_sc_soc driver
> in this patch series, so do you think we can add new SoC driver for i.MX8 SoC with SCU inside? Putting 2 different architecture
> SoCs' driver into 1 file looks like NOT making enough sense.
+1 for separate SOC driver. The 8mq/8mm and 8qm/8qxp families are very
different, they just happen to share the imx8 prefix.
It makes sense to allow people to compile one without the other and this
is easier with distinct SOC drivers.
--
Regards,
Leonard
On Mon, Apr 22, 2019 at 08:48:56AM +0000, Leonard Crestez wrote:
> On 4/22/2019 9:46 AM, Anson Huang wrote:
> >> -----Original Message-----
> >> From: Anson Huang
> >>> From: Shawn Guo [mailto:[email protected]]
> >>> On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
> >>>> On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> >>>>> 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:
> >>>>>
> >>>>> 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
> >>>>
> >>>> Rather than creating a new driver, please take a look at Abel's
> >>>> generic
> >>>> i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
> >>
> >> Got it, I didn't notice that this patch bas been accepted, I will redo the patch
> >> based on it, thanks.
> >
> > I have sent the new patch set to support i.MX8QXP SoC revision based on generic i.MX8
> > SoC driver, however, the Kconfig modification is NOT good, it may break i.MX8MQ if IMX_SCU
> > is NOT enabled, although we can add some warp function for SCU firmware API call to fix it,
> > but after further thought and discussion with Dong Aisheng, I think we may need to roll back to
> > use this patch series to create a new SoC driver dedicated for i.MX8 SoCs
> > with system controller inside, such as i.MX8QXP, i.MX8QM etc., the reason are as below:
> >
> > For i.MX8MQ/i.MX8MM:
> > 1. SoC driver does NOT depends on i.MX SCU firmware, so no need to use platform driver
> > probe model, just device_init phase call is good enough;
> > 2. The SoC driver no need to depends on IMX_SCU, so it can be always built in, no need to
> > check IMX_SCU config;
> > 3. The fuse check for CPU speed grading, HDCP status, NoC settings etc. could be added to this driver,
> > but they are ONLY for i.MX8MQ/i.MX8MM etc..
> > For i.MX8QXP/i.MX8QM:
> > 1. SoC driver MUST depends on IMX_SCU;
> > 2. MUST use platform model to support defer probe;
> > 3. No fuse check for CPU speed grading.
> >
> > So, I guess the reused code for i.MX8MQ and i.MX8QXP is ONLY those part of creating SoC id device node (less than
> > 30% I think), all other functions are implemented in total different ways, that is why I created the imx_sc_soc driver
> > in this patch series, so do you think we can add new SoC driver for i.MX8 SoC with SCU inside? Putting 2 different architecture
> > SoCs' driver into 1 file looks like NOT making enough sense.
>
> +1 for separate SOC driver. The 8mq/8mm and 8qm/8qxp families are very
> different, they just happen to share the imx8 prefix.
>
> It makes sense to allow people to compile one without the other and this
> is easier with distinct SOC drivers.
Leonard, Abel,
Can you guys help review the patch? Thanks.
Shawn
On 19-05-10 10:42:17, Shawn Guo wrote:
> On Mon, Apr 22, 2019 at 08:48:56AM +0000, Leonard Crestez wrote:
> > On 4/22/2019 9:46 AM, Anson Huang wrote:
> > >> -----Original Message-----
> > >> From: Anson Huang
> > >>> From: Shawn Guo [mailto:[email protected]]
> > >>> On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote:
> > >>>> On Thu, Apr 11, 2019 at 06:49:12AM +0000, Anson Huang wrote:
> > >>>>> 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:
> > >>>>>
> > >>>>> 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
> > >>>>
> > >>>> Rather than creating a new driver, please take a look at Abel's
> > >>>> generic
> > >>>> i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP.
> > >>
> > >> Got it, I didn't notice that this patch bas been accepted, I will redo the patch
> > >> based on it, thanks.
> > >
> > > I have sent the new patch set to support i.MX8QXP SoC revision based on generic i.MX8
> > > SoC driver, however, the Kconfig modification is NOT good, it may break i.MX8MQ if IMX_SCU
> > > is NOT enabled, although we can add some warp function for SCU firmware API call to fix it,
> > > but after further thought and discussion with Dong Aisheng, I think we may need to roll back to
> > > use this patch series to create a new SoC driver dedicated for i.MX8 SoCs
> > > with system controller inside, such as i.MX8QXP, i.MX8QM etc., the reason are as below:
> > >
> > > For i.MX8MQ/i.MX8MM:
> > > 1. SoC driver does NOT depends on i.MX SCU firmware, so no need to use platform driver
> > > probe model, just device_init phase call is good enough;
> > > 2. The SoC driver no need to depends on IMX_SCU, so it can be always built in, no need to
> > > check IMX_SCU config;
> > > 3. The fuse check for CPU speed grading, HDCP status, NoC settings etc. could be added to this driver,
> > > but they are ONLY for i.MX8MQ/i.MX8MM etc..
> > > For i.MX8QXP/i.MX8QM:
> > > 1. SoC driver MUST depends on IMX_SCU;
> > > 2. MUST use platform model to support defer probe;
> > > 3. No fuse check for CPU speed grading.
> > >
> > > So, I guess the reused code for i.MX8MQ and i.MX8QXP is ONLY those part of creating SoC id device node (less than
> > > 30% I think), all other functions are implemented in total different ways, that is why I created the imx_sc_soc driver
> > > in this patch series, so do you think we can add new SoC driver for i.MX8 SoC with SCU inside? Putting 2 different architecture
> > > SoCs' driver into 1 file looks like NOT making enough sense.
> >
> > +1 for separate SOC driver. The 8mq/8mm and 8qm/8qxp families are very
> > different, they just happen to share the imx8 prefix.
> >
> > It makes sense to allow people to compile one without the other and this
> > is easier with distinct SOC drivers.
Totally agree.
>
> Leonard, Abel,
>
> Can you guys help review the patch? Thanks.
>
> Shawn
Looks good to me.
Reviewed-by: Abel Vesa <[email protected]>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Andy Gross
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Abel
> Vesa <[email protected]>; [email protected]; linux-
> [email protected]
> Cc: dl-linux-imx <[email protected]>
> 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 <[email protected]>
> > ---
> > 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