[PATCH 0/4] Add i.MX system controller RTC driver

[PATCH 0/4] Add i.MX system controller RTC driver

[Anson Huang]

NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and secure RTC etc. management, Linux kernel
has to communicate with system controller via MU (message unit)
IPC to do RTC operation.

Since the RTC set time MUST to be done in secure EL3 mode (required
by system controller firmware) and ALARM functions needs to be done
with general MU IRQ handle, these are NOT ready NOW, so this patch
ONLY supports RTC read time for now.

Note that this patch set is based on [V4,5/5] defconfig: arm64: add imx8qxp support,
https://patchwork.kernel.org/patch/10677315/

Anson Huang (4):
  rtc: add i.MX system controller RTC support
  dt-bindings: rtc: add binding doc for i.MX system controller RTC
    driver
  defconfig: arm64: add i.MX system controller RTC support
  ARM64: dts: imx: add i.MX8QXP system controller RTC support

 .../devicetree/bindings/rtc/rtc-imx-sc.txt         |  10 ++
 arch/arm64/boot/dts/freescale/imx8qxp.dtsi         |   4 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/rtc/Kconfig                                |   6 ++
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-imx-sc.c                           | 104 +++++++++++++++++++++
 6 files changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
 create mode 100644 drivers/rtc/rtc-imx-sc.c

-- 
2.7.4

[PATCH 1/4] rtc: add i.MX system controller RTC support

[Anson Huang]

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 secure rtc etc..

This patch adds i.MX system controller RTC driver support,
Linux kernel has to communicate with system controller via MU
(message unit) IPC to set/get RTC time and other alarm functions,
since the RTC set time needs to be done in secure EL3 mode (required
by system controller firmware) and alarm functions needs to be done
with general MU IRQ handle, these depend on other components which
are NOT ready, so this patch ONLY enables the RTC time read.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/rtc/Kconfig      |   6 +++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-imx-sc.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 drivers/rtc/rtc-imx-sc.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a819ef0..3b9642e 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1677,6 +1677,12 @@ config RTC_DRV_SNVS
 	   This driver can also be built as a module, if so, the module
 	   will be called "rtc-snvs".
 
+config RTC_DRV_IMX_SC
+	tristate "NXP i.MX System Controller RTC support"
+	help
+	   If you say yes here you get support for the NXP i.MX System
+	   Controller RTC module.
+
 config RTC_DRV_SIRFSOC
 	tristate "SiRFSOC RTC"
 	depends on ARCH_SIRF
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 290c173..965725f 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_RTC_DRV_SC27XX)	+= rtc-sc27xx.o
 obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
 obj-$(CONFIG_RTC_DRV_SIRFSOC)	+= rtc-sirfsoc.o
 obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
+obj-$(CONFIG_RTC_DRV_IMX_SC)	+= rtc-imx-sc.o
 obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
 obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
 obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
diff --git a/drivers/rtc/rtc-imx-sc.c b/drivers/rtc/rtc-imx-sc.c
new file mode 100644
index 0000000..2466b55
--- /dev/null
+++ b/drivers/rtc/rtc-imx-sc.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#define IMX_SC_TIMER_FUNC_GET_RTC_SEC1970	9
+#define IMX_SC_TIMER_FUNC_SET_RTC_TIME		6
+
+struct imx_sc_ipc *rtc_ipc_handle;
+struct rtc_device *rtc;
+
+struct imx_sc_msg_req_timer_get_rtc_time {
+	struct imx_sc_rpc_msg hdr;
+} __packed;
+
+struct imx_sc_msg_resp_timer_get_rtc_time {
+	struct imx_sc_rpc_msg hdr;
+	u32 time;
+} __packed;
+
+static int imx_sc_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct imx_sc_msg_resp_timer_get_rtc_time *resp;
+	struct imx_sc_msg_req_timer_get_rtc_time msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_TIMER;
+	hdr->func = IMX_SC_TIMER_FUNC_GET_RTC_SEC1970;
+	hdr->size = 1;
+
+	ret = imx_scu_call_rpc(rtc_ipc_handle, &msg, true);
+	if (ret) {
+		pr_err("read rtc time failed, ret %d\n", ret);
+		return ret;
+	}
+
+	resp = (struct imx_sc_msg_resp_timer_get_rtc_time *)&msg;
+	rtc_time_to_tm(resp->time, tm);
+
+	return 0;
+}
+
+static const struct rtc_class_ops imx_sc_rtc_ops = {
+	.read_time = imx_sc_rtc_read_time,
+};
+
+static int imx_sc_rtc_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = imx_scu_get_handle(&rtc_ipc_handle);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		dev_err(&pdev->dev, "failed to get ipc handle: %d!\n", ret);
+		return ret;
+	}
+
+	rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
+					&imx_sc_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc)) {
+		ret = PTR_ERR(rtc);
+		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id imx_sc_dt_ids[] = {
+	{ .compatible = "nxp,imx8qxp-sc-rtc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, imx_sc_dt_ids);
+
+static struct platform_driver imx_sc_rtc_driver = {
+	.driver = {
+		.name	= "imx-sc-rtc",
+		.of_match_table = imx_sc_dt_ids,
+	},
+	.probe		= imx_sc_rtc_probe,
+};
+module_platform_driver(imx_sc_rtc_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("NXP i.MX System Controller RTC Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH 2/4] dt-bindings: rtc: add binding doc for i.MX system controller RTC driver

[Anson Huang]

NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and secure RTC etc. management, Linux kernel
has to communicate with system controller via MU (message unit)
IPC to do RTC operation, this patch adds binding doc for i.MX
system controller RTC driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt

diff --git a/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt b/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
new file mode 100644
index 0000000..d6e2353
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
@@ -0,0 +1,10 @@
+* NXP i.MX System Controller Real Time Clock
+
+Required properties:
+- compatible: should be "nxp,imx8qxp-sc-rtc";
+
+Example:
+
+rtc: rtc {
+	compatible = "nxp,imx8qxp-sc-rtc";
+};
-- 
2.7.4

[PATCH 3/4] defconfig: arm64: add i.MX system controller RTC support

[Anson Huang]

This patch enables CONFIG_RTC_DRV_IMX_SC by default.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 6d224f7..6fdf2d0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -573,6 +573,7 @@ CONFIG_RTC_DRV_PL031=y
 CONFIG_RTC_DRV_SUN6I=y
 CONFIG_RTC_DRV_ARMADA38X=y
 CONFIG_RTC_DRV_TEGRA=y
+CONFIG_RTC_DRV_IMX_SC=y
 CONFIG_RTC_DRV_XGENE=y
 CONFIG_DMADEVICES=y
 CONFIG_DMA_BCM2835=m
-- 
2.7.4

[PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Add i.MX8QXP system controller RTC support.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 9155d45..ef57db6 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -433,4 +433,8 @@
 		#size-cells = <1>;
 		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
 	};
+
+	rtc: rtc {
+		compatible = "nxp,imx8qxp-sc-rtc";
+	};
 };
-- 
2.7.4

Re: [PATCH 2/4] dt-bindings: rtc: add binding doc for i.MX system controller RTC driver

[Alexandre Belloni]

Hello,

On 27/11/2018 09:41:29+0000, Anson Huang wrote:
> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> system controller, the system controller is in charge of system
> power, clock and secure RTC etc. management, Linux kernel
> has to communicate with system controller via MU (message unit)
> IPC to do RTC operation, this patch adds binding doc for i.MX
> system controller RTC driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
> 

This patch should come first to silence checkpatch.

> diff --git a/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt b/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
> new file mode 100644
> index 0000000..d6e2353
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-imx-sc.txt
> @@ -0,0 +1,10 @@
> +* NXP i.MX System Controller Real Time Clock
> +
> +Required properties:
> +- compatible: should be "nxp,imx8qxp-sc-rtc";
> +
> +Example:
> +
> +rtc: rtc {
> +	compatible = "nxp,imx8qxp-sc-rtc";
> +};
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/4] rtc: add i.MX system controller RTC support

[Alexandre Belloni]

Hello,

Thank you for your submission.

On 27/11/2018 09:41:20+0000, Anson Huang wrote:
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 290c173..965725f 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -151,6 +151,7 @@ obj-$(CONFIG_RTC_DRV_SC27XX)	+= rtc-sc27xx.o
>  obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
>  obj-$(CONFIG_RTC_DRV_SIRFSOC)	+= rtc-sirfsoc.o
>  obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
> +obj-$(CONFIG_RTC_DRV_IMX_SC)	+= rtc-imx-sc.o

This list must be sorted alphabetically.

>  obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
>  obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
>  obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
> diff --git a/drivers/rtc/rtc-imx-sc.c b/drivers/rtc/rtc-imx-sc.c
> new file mode 100644
> index 0000000..2466b55
> --- /dev/null
> +++ b/drivers/rtc/rtc-imx-sc.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

As you have the SPDX identifier, I think this boiler plate license text
should be removed.

> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +
> +#define IMX_SC_TIMER_FUNC_GET_RTC_SEC1970	9
> +#define IMX_SC_TIMER_FUNC_SET_RTC_TIME		6
> +
> +struct imx_sc_ipc *rtc_ipc_handle;
> +struct rtc_device *rtc;
> +
> +struct imx_sc_msg_req_timer_get_rtc_time {
> +	struct imx_sc_rpc_msg hdr;
> +} __packed;
> +
> +struct imx_sc_msg_resp_timer_get_rtc_time {
> +	struct imx_sc_rpc_msg hdr;
> +	u32 time;
> +} __packed;
> +
> +static int imx_sc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct imx_sc_msg_resp_timer_get_rtc_time *resp;
> +	struct imx_sc_msg_req_timer_get_rtc_time msg;
> +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +	int ret;
> +
> +	hdr->ver = IMX_SC_RPC_VERSION;
> +	hdr->svc = IMX_SC_RPC_SVC_TIMER;
> +	hdr->func = IMX_SC_TIMER_FUNC_GET_RTC_SEC1970;
> +	hdr->size = 1;
> +
> +	ret = imx_scu_call_rpc(rtc_ipc_handle, &msg, true);
> +	if (ret) {
> +		pr_err("read rtc time failed, ret %d\n", ret);
> +		return ret;
> +	}
> +
> +	resp = (struct imx_sc_msg_resp_timer_get_rtc_time *)&msg;
> +	rtc_time_to_tm(resp->time, tm);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops imx_sc_rtc_ops = {
> +	.read_time = imx_sc_rtc_read_time,
> +};
> +
> +static int imx_sc_rtc_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	ret = imx_scu_get_handle(&rtc_ipc_handle);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		dev_err(&pdev->dev, "failed to get ipc handle: %d!\n", ret);
> +		return ret;
> +	}
> +
> +	rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +					&imx_sc_rtc_ops, THIS_MODULE);

Please use devm_rtc_allocate_device and rtc_register_device to register
the RTC. If possible, you should also set range_min and range_max. If I
understand the code correctly, this should simply be range_max = U32_MAX;


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/4] defconfig: arm64: add i.MX system controller RTC support

[Alexandre Belloni]

On 27/11/2018 09:41:37+0000, Anson Huang wrote:
> This patch enables CONFIG_RTC_DRV_IMX_SC by default.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 6d224f7..6fdf2d0 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -573,6 +573,7 @@ CONFIG_RTC_DRV_PL031=y
>  CONFIG_RTC_DRV_SUN6I=y
>  CONFIG_RTC_DRV_ARMADA38X=y
>  CONFIG_RTC_DRV_TEGRA=y
> +CONFIG_RTC_DRV_IMX_SC=y

I think this has to be m unless you need the RTC to be able to boot.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Lothar Waßmann]

Hi,

On Tue, 27 Nov 2018 09:41:46 +0000 Anson Huang wrote:
> Add i.MX8QXP system controller RTC support.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> index 9155d45..ef57db6 100644
> --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> @@ -433,4 +433,8 @@
>  		#size-cells = <1>;
>  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
>  	};
> +
> +	rtc: rtc {
> +		compatible = "nxp,imx8qxp-sc-rtc";
> +	};
>  };
IMO this should be disabled by default.


Lothar Waßmann

RE: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Hi, Lothar

Best Regards!
Anson Huang

> -----Original Message-----
> From: Lothar Waßmann [mailto:LW@KARO-electronics.de]
> Sent: 2018年11月28日 17:04
> To: Anson Huang <anson.huang@nxp.com>
> Cc: a.zummo@towertech.it; alexandre.belloni@bootlin.com;
> robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> RTC support
> 
> Hi,
> 
> On Tue, 27 Nov 2018 09:41:46 +0000 Anson Huang wrote:
> > Add i.MX8QXP system controller RTC support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > index 9155d45..ef57db6 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > @@ -433,4 +433,8 @@
> >  		#size-cells = <1>;
> >  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
> >  	};
> > +
> > +	rtc: rtc {
> > +		compatible = "nxp,imx8qxp-sc-rtc";
> > +	};
> >  };
> IMO this should be disabled by default.

This module is NOT depending on any board design, should it be enabled by default in soc dtsi?
Like wdog etc..

Anson.

> 
> 
> Lothar Waßmann

Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Lothar Waßmann]

Anson Huang <anson.huang@nxp.com> wrote:

> Hi, Lothar
> 
> Best Regards!
> Anson Huang
> 
> > -----Original Message-----
> > From: Lothar Waßmann [mailto:LW@KARO-electronics.de]
> > Sent: 2018年11月28日 17:04
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: a.zummo@towertech.it; alexandre.belloni@bootlin.com;
> > robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> > will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> > <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> > heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> > amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> > shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> > RTC support
> > 
> > Hi,
> > 
> > On Tue, 27 Nov 2018 09:41:46 +0000 Anson Huang wrote:  
> > > Add i.MX8QXP system controller RTC support.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi  
> > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi  
> > > index 9155d45..ef57db6 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > @@ -433,4 +433,8 @@
> > >  		#size-cells = <1>;
> > >  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
> > >  	};
> > > +
> > > +	rtc: rtc {
> > > +		compatible = "nxp,imx8qxp-sc-rtc";
> > > +	};
> > >  };  
> > IMO this should be disabled by default.  
> 
> This module is NOT depending on any board design, should it be enabled by default in soc dtsi?
> Like wdog etc..
> 
What about existing users (if there are any), which would have a new
device appearing when updating the DTB?


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Alexandre Belloni]

On 28/11/2018 09:06:27+0000, Anson Huang wrote:
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > index 9155d45..ef57db6 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > @@ -433,4 +433,8 @@
> > >  		#size-cells = <1>;
> > >  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
> > >  	};
> > > +
> > > +	rtc: rtc {
> > > +		compatible = "nxp,imx8qxp-sc-rtc";
> > > +	};
> > >  };
> > IMO this should be disabled by default.
> 
> This module is NOT depending on any board design, should it be enabled by default in soc dtsi?
> Like wdog etc..
> 

I guess the point is that you may not want it enabled, even if it should
always be working. enabling it by default would affect the RTC ordering
for example.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Hi, Lothar

Best Regards!
Anson Huang

> -----Original Message-----
> From: Lothar Waßmann [mailto:LW@KARO-electronics.de]
> Sent: 2018年11月28日 17:13
> To: Anson Huang <anson.huang@nxp.com>
> Cc: a.zummo@towertech.it; alexandre.belloni@bootlin.com;
> robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> RTC support
> 
> Anson Huang <anson.huang@nxp.com> wrote:
> 
> > Hi, Lothar
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Lothar Waßmann [mailto:LW@KARO-electronics.de]
> > > Sent: 2018年11月28日 17:04
> > > To: Anson Huang <anson.huang@nxp.com>
> > > Cc: a.zummo@towertech.it; alexandre.belloni@bootlin.com;
> > > robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> > > will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy
> Gross
> > > <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> > > heiko@sntech.de; bjorn.andersson@linaro.org;
> > > enric.balletbo@collabora.com; amit.kucheria@linaro.org;
> > > ezequiel@collabora.com; olof@lixom.net; shawnguo@kernel.org;
> > > linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system
> > > controller RTC support
> > >
> > > Hi,
> > >
> > > On Tue, 27 Nov 2018 09:41:46 +0000 Anson Huang wrote:
> > > > Add i.MX8QXP system controller RTC support.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > >  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > index 9155d45..ef57db6 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > @@ -433,4 +433,8 @@
> > > >  		#size-cells = <1>;
> > > >  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
> > > >  	};
> > > > +
> > > > +	rtc: rtc {
> > > > +		compatible = "nxp,imx8qxp-sc-rtc";
> > > > +	};
> > > >  };
> > > IMO this should be disabled by default.
> >
> > This module is NOT depending on any board design, should it be enabled by
> default in soc dtsi?
> > Like wdog etc..
> >
> What about existing users (if there are any), which would have a new device
> appearing when updating the DTB?

This i.MX8QXP is a new SoC on the way to upstream, no old dtb used, so is it OK to keep it enabled
by default and aligned with all previous i.MX SoCs' RTC implementation? If we disable it
here, we have to enable it in all boards dtb. So if it is NOT very critical, is it OK to keep
it enabled by default here? Thanks.

Anson

> 
> 
> Lothar Waßmann
> --
> ___________________________________________________________
> 
> Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
> Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
> Geschäftsführer: Matthias Kaussen
> Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
> 
> https://emea01.safelinks.protection.outlook.com/?url=www.karo-electronics.d
> e&amp;data=02%7C01%7Canson.huang%40nxp.com%7C12fb8e546a0e46d03
> 72408d65511a43f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 636789931600483807&amp;sdata=SVZjpvYsi9XAM7CGt8sHZ5mt8rPRa%2FPl
> 75EQvHTod%2Bg%3D&amp;reserved=0 | info@karo-electronics.de
> ___________________________________________________________

RE: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@bootlin.com]
> Sent: 2018年11月28日 17:20
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Lothar Waßmann <LW@KARO-electronics.de>; a.zummo@towertech.it;
> robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> RTC support
> 
> On 28/11/2018 09:06:27+0000, Anson Huang wrote:
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > index 9155d45..ef57db6 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > > @@ -433,4 +433,8 @@
> > > >  		#size-cells = <1>;
> > > >  		ranges = <0x5f000000 0x0 0x5f000000 0x1000000>;
> > > >  	};
> > > > +
> > > > +	rtc: rtc {
> > > > +		compatible = "nxp,imx8qxp-sc-rtc";
> > > > +	};
> > > >  };
> > > IMO this should be disabled by default.
> >
> > This module is NOT depending on any board design, should it be enabled by
> default in soc dtsi?
> > Like wdog etc..
> >
> 
> I guess the point is that you may not want it enabled, even if it should always
> be working. enabling it by default would affect the RTC ordering for example.

i.MX SoCs ONLY have 1 RTC. And RTC are always enabled by default for all i.MX
SoCs, so do we have to disable it here and enable it in board dts?

Anson.

> 
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootl
> in.com&amp;data=02%7C01%7Canson.huang%40nxp.com%7C0586fd10d6ff4
> 5d03f4308d65512a864%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636789935951215703&amp;sdata=Hs1AXXoZMMKl8inOXFs8xXNXQ9EJG
> gtphyTNhFvMiTE%3D&amp;reserved=0

Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Alexandre Belloni]

On 28/11/2018 09:21:56+0000, Anson Huang wrote:
> > > This module is NOT depending on any board design, should it be enabled by
> > default in soc dtsi?
> > > Like wdog etc..
> > >
> > 
> > I guess the point is that you may not want it enabled, even if it should always
> > be working. enabling it by default would affect the RTC ordering for example.
> 
> i.MX SoCs ONLY have 1 RTC. And RTC are always enabled by default for all i.MX
> SoCs, so do we have to disable it here and enable it in board dts?
> 

I would say that most of the i.MX based boards include another RTC
because the SoC one consumes way too much power.

Note that I don't care too much whether it is enabled by default, I
was simply explaining why you may want to disable it by default.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@bootlin.com]
> Sent: 2018年11月28日 17:34
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Lothar Wa��mann <LW@KARO-electronics.de>; a.zummo@towertech.it;
> robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> RTC support
> 
> On 28/11/2018 09:21:56+0000, Anson Huang wrote:
> > > > This module is NOT depending on any board design, should it be
> > > > enabled by
> > > default in soc dtsi?
> > > > Like wdog etc..
> > > >
> > >
> > > I guess the point is that you may not want it enabled, even if it
> > > should always be working. enabling it by default would affect the RTC
> ordering for example.
> >
> > i.MX SoCs ONLY have 1 RTC. And RTC are always enabled by default for
> > all i.MX SoCs, so do we have to disable it here and enable it in board dts?
> >
> 
> I would say that most of the i.MX based boards include another RTC because
> the SoC one consumes way too much power.
> 
> Note that I don't care too much whether it is enabled by default, I was simply
> explaining why you may want to disable it by default.

OK, this system controller RTC is a little different, it is controlled by system controller firmware,
and system controller firmware will always select the best one for its user(Linux kernel) if there are other
RTCs available, so I think we can keep it enabled by default for now. Thanks.

Anson.

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootl
> in.com&amp;data=02%7C01%7Canson.huang%40nxp.com%7C5be6d014f2a5
> 4876cc9408d65514eaf3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C636789945664646785&amp;sdata=MW0xy3cUhCLBTrEFkk4vAylxSAwB
> mx1Ws6ZeEhAsdME%3D&amp;reserved=0

Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Alexandre Belloni]

On 28/11/2018 09:41:22+0000, Anson Huang wrote:
> > > i.MX SoCs ONLY have 1 RTC. And RTC are always enabled by default for
> > > all i.MX SoCs, so do we have to disable it here and enable it in board dts?
> > >
> > 
> > I would say that most of the i.MX based boards include another RTC because
> > the SoC one consumes way too much power.
> > 
> > Note that I don't care too much whether it is enabled by default, I was simply
> > explaining why you may want to disable it by default.
> 
> OK, this system controller RTC is a little different, it is controlled by system controller firmware,
> and system controller firmware will always select the best one for its user(Linux kernel) if there are other
> RTCs available, so I think we can keep it enabled by default for now. Thanks.
> 

Do you mean that the plan is to push support for the external RTCs (e.g.
the i2c ones) to the M4 firmware?

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller RTC support

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@bootlin.com]
> Sent: 2018年11月28日 17:58
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Lothar Wa��mann <LW@KARO-electronics.de>; a.zummo@towertech.it;
> robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Aisheng DONG <aisheng.dong@nxp.com>; Andy Gross
> <andy.gross@linaro.org>; arnd@arndb.de; horms+renesas@verge.net.au;
> heiko@sntech.de; bjorn.andersson@linaro.org; enric.balletbo@collabora.com;
> amit.kucheria@linaro.org; ezequiel@collabora.com; olof@lixom.net;
> shawnguo@kernel.org; linux-rtc@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/4] ARM64: dts: imx: add i.MX8QXP system controller
> RTC support
> 
> On 28/11/2018 09:41:22+0000, Anson Huang wrote:
> > > > i.MX SoCs ONLY have 1 RTC. And RTC are always enabled by default
> > > > for all i.MX SoCs, so do we have to disable it here and enable it in board
> dts?
> > > >
> > >
> > > I would say that most of the i.MX based boards include another RTC
> > > because the SoC one consumes way too much power.
> > >
> > > Note that I don't care too much whether it is enabled by default, I
> > > was simply explaining why you may want to disable it by default.
> >
> > OK, this system controller RTC is a little different, it is controlled
> > by system controller firmware, and system controller firmware will
> > always select the best one for its user(Linux kernel) if there are other RTCs
> available, so I think we can keep it enabled by default for now. Thanks.
> >
> 
> Do you mean that the plan is to push support for the external RTCs (e.g.
> the i2c ones) to the M4 firmware?

As far as I know, currently there is no such plan/support in system controller (M4) firmware,
and if there are external RTCs, like some PMICs have RTC inside, yes, M4 will
control it and Linux kernel (AP) does NOT need to care about which RTC is used,
same IPC API will be used I think.

Anson.

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootl
> in.com&amp;data=02%7C01%7Canson.huang%40nxp.com%7C015fe8a15f364
> 066a6a608d655181d03%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C636789959398815495&amp;sdata=wmgVepaMjZyc7dJQ0x8cLSaQUlshs
> 6jKCjBdgC4FoYg%3D&amp;reserved=0