LinuxLists.cc - [PATCH v2 0/5] add mxc driver for i.MX53 SRTC

[permalink] [raw]

Subject: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

From: Patrick Bruenn <[email protected]>

Document the binding for i.MX53 SRTC implemented by rtc-mxc_v2

Signed-off-by: Patrick Bruenn <[email protected]><Paste>

---

To: Alessandro Zummo <[email protected]>
To: Alexandre Belloni <[email protected]>

Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list)
Cc: Fabio Estevam <[email protected]>
Cc: Juergen Borleis <[email protected]>
Cc: Noel Vellemans <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Cc: Russell King <[email protected]> (maintainer:ARM PORT)
Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <[email protected]>
Cc: Lothar Waßmann <[email protected]>
---
Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt

diff --git a/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
new file mode 100644
index 000000000000..796e7f4995db
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
@@ -0,0 +1,17 @@
+* i.MX53 Real Time Clock controller
+
+Required properties:
+- compatible: should be: "fsl,imx53-rtc"
+- reg: physical base address of the controller and length of memory mapped
+ region.
+- clocks: should contain the phandle for the rtc clock
+- interrupts: rtc alarm interrupt
+
+Example:
+
+srtc@53fa4000 {
+ compatible = "fsl,imx53-rtc";
+ reg = <0x53fa4000 0x4000>;
+ interrupts = <24>;
+ clocks = <&clks IMX5_CLK_SRTC_GATE>;
+};
--
2.11.0

2017-12-05 14:08:32

[permalink] [raw]

Subject: [PATCH v2 3/5] rtc: mxc_v2: add driver for i.MX53 SRTC

From: Patrick Bruenn <[email protected]>

Add RTC_DRV_MXC_V2 config option

Signed-off-by: Patrick Bruenn <[email protected]>

---

To: Alessandro Zummo <[email protected]>
To: Alexandre Belloni <[email protected]>

Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list)
Cc: Fabio Estevam <[email protected]>
Cc: Juergen Borleis <[email protected]>
Cc: Noel Vellemans <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Cc: Russell King <[email protected]> (maintainer:ARM PORT)
Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <[email protected]>
Cc: Lothar Waßmann <[email protected]>
---
drivers/rtc/Kconfig | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b59a31b079a5..440edebf5c71 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1689,6 +1689,16 @@ config RTC_DRV_MXC
This driver can also be built as a module, if so, the module
will be called "rtc-mxc".

+config RTC_DRV_MXC_V2
+ tristate "Freescale MXC Real Time Clock for i.MX53"
+ depends on ARCH_MXC
+ help
+ If you say yes here you get support for the Freescale MXC
+ SRTC module in i.MX53 processor.
+
+ This driver can also be built as a module, if so, the module
+ will be called "rtc-mxc_v2".
+
config RTC_DRV_SNVS
tristate "Freescale SNVS RTC support"
select REGMAP_MMIO
--
2.11.0

2017-12-05 14:08:25

[permalink] [raw]

Subject: [PATCH v2 4/5] ARM: imx_v4_v5_defconfig: enable RTC_DRV_MXC_V2

From: Patrick Bruenn <[email protected]>

Enable SRTC driver for i.MX53 in default config

Signed-off-by: Patrick Bruenn <[email protected]>

---

To: Shawn Guo <[email protected]>
To: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list)
Cc: Fabio Estevam <[email protected]>
Cc: Juergen Borleis <[email protected]>
Cc: Noel Vellemans <[email protected]>
Cc: Russell King <[email protected]> (maintainer:ARM PORT)
Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <[email protected]>
Cc: Lothar Waßmann <[email protected]>
---
arch/arm/configs/imx_v4_v5_defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v4_v5_defconfig b/arch/arm/configs/imx_v4_v5_defconfig
index ca0f13cafe38..b48efd5ff8a7 100644
--- a/arch/arm/configs/imx_v4_v5_defconfig
+++ b/arch/arm/configs/imx_v4_v5_defconfig
@@ -167,6 +167,7 @@ CONFIG_RTC_DRV_PCF8563=y
CONFIG_RTC_DRV_IMXDI=y
CONFIG_RTC_DRV_MC13XXX=y
CONFIG_RTC_DRV_MXC=y
+CONFIG_RTC_DRV_MXC_V2=y
CONFIG_DMADEVICES=y
CONFIG_IMX_DMA=y
CONFIG_IMX_SDMA=y
--
2.11.0

2017-12-05 14:08:22

[permalink] [raw]

Subject: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

From: Patrick Bruenn <[email protected]>

Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53.

This is driver enables support for the low power domain SRTC features:
- 32-bit MSB of non-rollover time counter
- 32-bit alarm register

Based on:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01

Signed-off-by: Patrick Bruenn <[email protected]>

---

v2:
- have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver
- add SPDX-License-Identifier and cleanup copyright notice
- replace __raw_readl/writel() with readl/writel()
- fix PM_SLEEP callbacks
- add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c
- remove misleading or obvious comments and fix style of the remaining
- avoid endless loop while waiting for hw
- implement consistent locking; make spinlock a member of dev struct
- enable clk only for register accesses
- remove all udelay() calls since they are obsolete or redundant
(we are already waiting for register flags to change)
- init platform_data before registering irq callback
- let set_time() fail, when 32 bit rtc counter exceeded
- make names more consistent
- cleanup and reorder includes
- cleanup and remove unused defines

To: Alessandro Zummo <[email protected]>
To: Alexandre Belloni <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: [email protected] (open list)
Cc: Fabio Estevam <[email protected]>
Cc: Juergen Borleis <[email protected]>
Cc: Noel Vellemans <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Cc: Russell King <[email protected]> (maintainer:ARM PORT)
Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <[email protected]>
Cc: Lothar Waßmann <[email protected]>
---
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-mxc_v2.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 434 insertions(+)
create mode 100644 drivers/rtc/rtc-mxc_v2.c

diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index f2f50c11dc38..dcf60e61ae5c 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o
+obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o
obj-$(CONFIG_RTC_DRV_NUC900) += rtc-nuc900.o
obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o
obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o
diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c
new file mode 100644
index 000000000000..c5a6d2c293bb
--- /dev/null
+++ b/drivers/rtc/rtc-mxc_v2.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Real Time Clock (RTC) Driver for i.MX53
+ * Copyright (c) 2004-2011 Freescale Semiconductor, Inc.
+ * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */
+
+#define SRTC_LPCR_EN_LP BIT(3) /* lp enable */
+#define SRTC_LPCR_WAE BIT(4) /* lp wakeup alarm enable */
+#define SRTC_LPCR_ALP BIT(7) /* lp alarm flag */
+#define SRTC_LPCR_NSA BIT(11) /* lp non secure access */
+#define SRTC_LPCR_NVE BIT(14) /* lp non valid state exit bit */
+#define SRTC_LPCR_IE BIT(15) /* lp init state exit bit */
+
+#define SRTC_LPSR_ALP BIT(3) /* lp alarm flag */
+#define SRTC_LPSR_NVES BIT(14) /* lp non-valid state exit status */
+#define SRTC_LPSR_IES BIT(15) /* lp init state exit status */
+
+#define SRTC_LPSCMR 0x00 /* LP Secure Counter MSB Reg */
+#define SRTC_LPSCLR 0x04 /* LP Secure Counter LSB Reg */
+#define SRTC_LPSAR 0x08 /* LP Secure Alarm Reg */
+#define SRTC_LPCR 0x10 /* LP Control Reg */
+#define SRTC_LPSR 0x14 /* LP Status Reg */
+#define SRTC_LPPDR 0x18 /* LP Power Supply Glitch Detector Reg */
+
+/* max. number of retries to read registers, 120 was max during test */
+#define REG_READ_TIMEOUT 2000
+
+struct mxc_rtc_data {
+ struct rtc_device *rtc;
+ void __iomem *ioaddr;
+ struct clk *clk;
+ spinlock_t lock; /* protects register access */
+ int irq;
+};
+
+/*
+ * This function does write synchronization for writes to the lp srtc block.
+ * To take care of the asynchronous CKIL clock, all writes from the IP domain
+ * will be synchronized to the CKIL domain.
+ * The caller should hold the pdata->lock
+ */
+static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr)
+{
+ unsigned int i;
+
+ /* Wait for 3 CKIL cycles */
+ for (i = 0; i < 3; i++) {
+ const u32 count = readl(ioaddr + SRTC_LPSCLR);
+ unsigned int timeout = REG_READ_TIMEOUT;
+
+ while ((readl(ioaddr + SRTC_LPSCLR)) == count) {
+ if (!--timeout) {
+ pr_err("SRTC_LPSCLR stuck! Check your hw.\n");
+ return;
+ }
+ }
+ }
+}
+
+/*
+ * This function updates the RTC alarm registers and then clears all the
+ * interrupt status bits.
+ * The caller should hold the pdata->lock
+ *
+ * @param alrm the new alarm value to be updated in the RTC
+ *
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
+ struct rtc_time *alarm_tm)
+{
+ void __iomem *const ioaddr = pdata->ioaddr;
+ unsigned long time;
+
+ rtc_tm_to_time(alarm_tm, &time);
+
+ if (time > U32_MAX) {
+ pr_err("Hopefully I am out of service by then :-(\n");
+ return -EINVAL;
+ }
+
+ writel((u32)time, ioaddr + SRTC_LPSAR);
+
+ /* clear alarm interrupt status bit */
+ writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
+
+ mxc_rtc_sync_lp_locked(ioaddr);
+ return 0;
+}
+
+/* This function is the RTC interrupt service routine. */
+static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
+{
+ struct platform_device *pdev = dev_id;
+ struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
+ void __iomem *ioaddr = pdata->ioaddr;
+ unsigned long flags;
+ u32 events = 0;
+ u32 lp_status;
+ u32 lp_cr;
+
+ spin_lock_irqsave(&pdata->lock, flags);
+ if (clk_prepare_enable(pdata->clk)) {
+ spin_unlock_irqrestore(&pdata->lock, flags);
+ return IRQ_NONE;
+ }
+
+ lp_status = readl(ioaddr + SRTC_LPSR);
+ lp_cr = readl(ioaddr + SRTC_LPCR);
+
+ /* update irq data & counter */
+ if (lp_status & SRTC_LPSR_ALP) {
+ if (lp_cr & SRTC_LPCR_ALP)
+ events = (RTC_AF | RTC_IRQF);
+
+ /* disable further lp alarm interrupts */
+ lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+ }
+
+ /* Update interrupt enables */
+ writel(lp_cr, ioaddr + SRTC_LPCR);
+
+ /* clear interrupt status */
+ writel(lp_status, ioaddr + SRTC_LPSR);
+
+ mxc_rtc_sync_lp_locked(ioaddr);
+ rtc_update_irq(pdata->rtc, 1, events);
+ clk_disable_unprepare(pdata->clk);
+ spin_unlock_irqrestore(&pdata->lock, flags);
+ return IRQ_HANDLED;
+}
+
+/*
+ * Enable clk and aquire spinlock
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_lock(struct mxc_rtc_data *const pdata)
+{
+ int ret;
+
+ spin_lock_irq(&pdata->lock);
+ ret = clk_prepare_enable(pdata->clk);
+ if (ret) {
+ spin_unlock_irq(&pdata->lock);
+ return ret;
+ }
+ return 0;
+}
+
+static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata)
+{
+ clk_disable_unprepare(pdata->clk);
+ spin_unlock_irq(&pdata->lock);
+ return 0;
+}
+
+/*
+ * This function reads the current RTC time into tm in Gregorian date.
+ *
+ * @param tm contains the RTC time value upon return
+ *
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+ time_t now;
+ int ret = mxc_rtc_lock(pdata);
+
+ if (ret)
+ return ret;
+
+ now = readl(pdata->ioaddr + SRTC_LPSCMR);
+ rtc_time_to_tm(now, tm);
+ ret = rtc_valid_tm(tm);
+ mxc_rtc_unlock(pdata);
+ return ret;
+}
+
+/*
+ * This function sets the internal RTC time based on tm in Gregorian date.
+ *
+ * @param tm the time value to be set in the RTC
+ *
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+ time64_t time = rtc_tm_to_time64(tm);
+ int ret;
+
+ if (time > U32_MAX) {
+ dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX);
+ return -EINVAL;
+ }
+
+ ret = mxc_rtc_lock(pdata);
+ if (ret)
+ return ret;
+
+ writel(time, pdata->ioaddr + SRTC_LPSCMR);
+ mxc_rtc_sync_lp_locked(pdata->ioaddr);
+ return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * This function reads the current alarm value into the passed in \b alrm
+ * argument. It updates the \b alrm's pending field value based on the whether
+ * an alarm interrupt occurs or not.
+ *
+ * @param alrm contains the RTC alarm value upon return
+ *
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+ void __iomem *ioaddr = pdata->ioaddr;
+ int ret;
+
+ ret = mxc_rtc_lock(pdata);
+ if (ret)
+ return ret;
+
+ rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time);
+ alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP);
+ return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * Enable/Disable alarm interrupt
+ * The caller should hold the pdata->lock
+ */
+static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata,
+ unsigned int enable)
+{
+ u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR);
+
+ if (enable)
+ lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+ else
+ lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+
+ writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
+}
+
+static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+ int ret = mxc_rtc_lock(pdata);
+
+ if (ret)
+ return ret;
+
+ mxc_rtc_alarm_irq_enable_locked(pdata, enable);
+ return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * This function sets the RTC alarm based on passed in alrm.
+ *
+ * @param alrm the alarm value to be set in the RTC
+ *
+ * @return 0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+ int ret = mxc_rtc_lock(pdata);
+
+ if (ret)
+ return ret;
+
+ ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);
+ if (!ret) {
+ mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
+ mxc_rtc_sync_lp_locked(pdata->ioaddr);
+ }
+ mxc_rtc_unlock(pdata);
+ return ret;
+}
+
+static const struct rtc_class_ops mxc_rtc_ops = {
+ .read_time = mxc_rtc_read_time,
+ .set_time = mxc_rtc_set_time,
+ .read_alarm = mxc_rtc_read_alarm,
+ .set_alarm = mxc_rtc_set_alarm,
+ .alarm_irq_enable = mxc_rtc_alarm_irq_enable,
+};
+
+static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
+{
+ unsigned int timeout = REG_READ_TIMEOUT;
+
+ while (!(readl(ioaddr) & flag)) {
+ if (!--timeout) {
+ pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
+ return -EBUSY;
+ }
+ }
+ return 0;
+}
+
+static int mxc_rtc_probe(struct platform_device *pdev)
+{
+ struct mxc_rtc_data *pdata;
+ struct resource *res;
+ void __iomem *ioaddr;
+ int ret = 0;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(pdata->ioaddr))
+ return PTR_ERR(pdata->ioaddr);
+
+ ioaddr = pdata->ioaddr;
+
+ pdata->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pdata->clk)) {
+ dev_err(&pdev->dev, "unable to get rtc clock!\n");
+ return PTR_ERR(pdata->clk);
+ }
+
+ spin_lock_init(&pdata->lock);
+ pdata->irq = platform_get_irq(pdev, 0);
+ if (pdata->irq < 0)
+ return pdata->irq;
+
+ device_init_wakeup(&pdev->dev, 1);
+
+ ret = clk_prepare_enable(pdata->clk);
+ if (ret)
+ return ret;
+ /* initialize glitch detect */
+ writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
+
+ /* clear lp interrupt status */
+ writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
+
+ /* move out of init state */
+ writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
+ mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);
+
+ /* move out of non-valid state */
+ writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
+ SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
+ mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);
+
+ clk_disable_unprepare(pdata->clk);
+ platform_set_drvdata(pdev, pdata);
+ ret =
+ devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, 0,
+ pdev->name, pdev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "interrupt not available.\n");
+ return ret;
+ }
+
+ pdata->rtc =
+ devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
+ THIS_MODULE);
+ if (IS_ERR(pdata->rtc))
+ return PTR_ERR(pdata->rtc);
+
+ return 0;
+}
+
+static int __exit mxc_rtc_remove(struct platform_device *pdev)
+{
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mxc_rtc_suspend(struct device *dev)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev))
+ enable_irq_wake(pdata->irq);
+
+ return 0;
+}
+
+static int mxc_rtc_resume(struct device *dev)
+{
+ struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev))
+ disable_irq_wake(pdata->irq);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume);
+
+static const struct of_device_id mxc_ids[] = {
+ { .compatible = "fsl,imx53-rtc", },
+ {}
+};
+
+static struct platform_driver mxc_rtc_driver = {
+ .driver = {
+ .name = "mxc_rtc_v2",
+ .of_match_table = mxc_ids,
+ .pm = &mxc_rtc_pm_ops,
+ },
+ .probe = mxc_rtc_probe,
+ .remove = mxc_rtc_remove,
+};
+
+module_platform_driver(mxc_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Real Time Clock (RTC) Driver for i.MX53");
+MODULE_LICENSE("GPL");
--
2.11.0

2017-12-05 14:10:44

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] ARM: imx_v4_v5_defconfig: enable RTC_DRV_MXC_V2

Hi Patrick,

On Tue, Dec 5, 2017 at 12:06 PM, <[email protected]> wrote:

> arch/arm/configs/imx_v4_v5_defconfig | 1 +

i.mx53 uses imx_v6_v7_defconfig, not imx_v4_v5_defconfig.

2017-12-05 14:12:15

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] rtc: mxc_v2: add driver for i.MX53 SRTC

On Tue, Dec 5, 2017 at 12:06 PM, <[email protected]> wrote:
> From: Patrick Bruenn <[email protected]>
>
> Add RTC_DRV_MXC_V2 config option
>
> Signed-off-by: Patrick Bruenn <[email protected]>

You should add the Kconfig as part of the patch that introduces the driver.

2017-12-05 14:13:40

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

On Tue, Dec 5, 2017 at 12:06 PM, <[email protected]> wrote:

> srtc: srtc@53fa4000 {
> - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
> + compatible = "fsl,imx53-rtc";
> reg = <0x53fa4000 0x4000>;
> interrupts = <24>;
> - interrupt-parent = <&tzic>;
> clocks = <&clks IMX5_CLK_SRTC_GATE>;
> - clock-names = "ipg";

srtc node has been removed in linux-next.

You need to re-add it.

2017-12-05 14:20:45

[permalink] [raw]

Subject: RE: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

>From: Fabio Estevam [mailto:[email protected]]
>Sent: Dienstag, 5. Dezember 2017 15:14
>On Tue, Dec 5, 2017 at 12:06 PM, <[email protected]> wrote:
>
>> srtc: srtc@53fa4000 {
>> - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
>> + compatible = "fsl,imx53-rtc";
>> reg = <0x53fa4000 0x4000>;
>> interrupts = <24>;
>> - interrupt-parent = <&tzic>;
>> clocks = <&clks IMX5_CLK_SRTC_GATE>;
>> - clock-names = "ipg";
>
>srtc node has been removed in linux-next.
>
>You need to re-add it.
Thanks for this super-fast responses!
I will wait a few days for more reviewers and then integrate your comments in a v3. If nothing major show up I will wait until the imx53.dtsi revert landed in Linus tree.

Best regards,
Patrick

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

2017-12-06 08:36:38

by Sascha Hauer

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

On Tue, Dec 05, 2017 at 03:06:46PM +0100, [email protected] wrote:
> From: Patrick Bruenn <[email protected]>
>
> Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53.
>
> This is driver enables support for the low power domain SRTC features:
> - 32-bit MSB of non-rollover time counter
> - 32-bit alarm register
>
> Based on:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01
>
> Signed-off-by: Patrick Bruenn <[email protected]>
>
> ---
>
> v2:
> - have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver
> - add SPDX-License-Identifier and cleanup copyright notice
> - replace __raw_readl/writel() with readl/writel()
> - fix PM_SLEEP callbacks
> - add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c
> - remove misleading or obvious comments and fix style of the remaining
> - avoid endless loop while waiting for hw
> - implement consistent locking; make spinlock a member of dev struct
> - enable clk only for register accesses
> - remove all udelay() calls since they are obsolete or redundant
> (we are already waiting for register flags to change)
> - init platform_data before registering irq callback
> - let set_time() fail, when 32 bit rtc counter exceeded
> - make names more consistent
> - cleanup and reorder includes
> - cleanup and remove unused defines
>
> To: Alessandro Zummo <[email protected]>
> To: Alexandre Belloni <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: [email protected] (open list)
> Cc: Fabio Estevam <[email protected]>
> Cc: Juergen Borleis <[email protected]>
> Cc: Noel Vellemans <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <[email protected]> (maintainer:ARM PORT)
> Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
>
> Cc: Philippe Ombredanne <[email protected]>
> Cc: Lothar Wa?mann <[email protected]>
> ---
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-mxc_v2.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 434 insertions(+)
> create mode 100644 drivers/rtc/rtc-mxc_v2.c
>
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index f2f50c11dc38..dcf60e61ae5c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
> obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
> obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
> obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o
> +obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o
> obj-$(CONFIG_RTC_DRV_NUC900) += rtc-nuc900.o
> obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o
> obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o
> diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c
> new file mode 100644
> index 000000000000..c5a6d2c293bb
> --- /dev/null
> +++ b/drivers/rtc/rtc-mxc_v2.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Real Time Clock (RTC) Driver for i.MX53
> + * Copyright (c) 2004-2011 Freescale Semiconductor, Inc.
> + * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +
> +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */
> +
> +#define SRTC_LPCR_EN_LP BIT(3) /* lp enable */
> +#define SRTC_LPCR_WAE BIT(4) /* lp wakeup alarm enable */
> +#define SRTC_LPCR_ALP BIT(7) /* lp alarm flag */
> +#define SRTC_LPCR_NSA BIT(11) /* lp non secure access */
> +#define SRTC_LPCR_NVE BIT(14) /* lp non valid state exit bit */
> +#define SRTC_LPCR_IE BIT(15) /* lp init state exit bit */
> +
> +#define SRTC_LPSR_ALP BIT(3) /* lp alarm flag */
> +#define SRTC_LPSR_NVES BIT(14) /* lp non-valid state exit status */
> +#define SRTC_LPSR_IES BIT(15) /* lp init state exit status */
> +
> +#define SRTC_LPSCMR 0x00 /* LP Secure Counter MSB Reg */
> +#define SRTC_LPSCLR 0x04 /* LP Secure Counter LSB Reg */
> +#define SRTC_LPSAR 0x08 /* LP Secure Alarm Reg */
> +#define SRTC_LPCR 0x10 /* LP Control Reg */
> +#define SRTC_LPSR 0x14 /* LP Status Reg */
> +#define SRTC_LPPDR 0x18 /* LP Power Supply Glitch Detector Reg */
> +
> +/* max. number of retries to read registers, 120 was max during test */
> +#define REG_READ_TIMEOUT 2000
> +
> +struct mxc_rtc_data {
> + struct rtc_device *rtc;
> + void __iomem *ioaddr;
> + struct clk *clk;
> + spinlock_t lock; /* protects register access */
> + int irq;
> +};
> +
> +/*
> + * This function does write synchronization for writes to the lp srtc block.
> + * To take care of the asynchronous CKIL clock, all writes from the IP domain
> + * will be synchronized to the CKIL domain.
> + * The caller should hold the pdata->lock
> + */
> +static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr)
> +{
> + unsigned int i;
> +
> + /* Wait for 3 CKIL cycles */
> + for (i = 0; i < 3; i++) {
> + const u32 count = readl(ioaddr + SRTC_LPSCLR);
> + unsigned int timeout = REG_READ_TIMEOUT;
> +
> + while ((readl(ioaddr + SRTC_LPSCLR)) == count) {
> + if (!--timeout) {
> + pr_err("SRTC_LPSCLR stuck! Check your hw.\n");
> + return;
> + }
> + }
> + }
> +}
> +
> +/*
> + * This function updates the RTC alarm registers and then clears all the
> + * interrupt status bits.
> + * The caller should hold the pdata->lock
> + *
> + * @param alrm the new alarm value to be updated in the RTC
> + *
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
> + struct rtc_time *alarm_tm)
> +{
> + void __iomem *const ioaddr = pdata->ioaddr;
> + unsigned long time;
> +
> + rtc_tm_to_time(alarm_tm, &time);
> +
> + if (time > U32_MAX) {
> + pr_err("Hopefully I am out of service by then :-(\n");
> + return -EINVAL;
> + }

This will never happen as on your target hardware unsigned long is a
32bit type. Not sure what is best to do here. Maybe you should test
the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
but rtc_tm_to_time could detect when the input time doesn't fit into
its return type and return an error in this case.
Also I just realized that it's unsigned and only overflows in the year
2106. I'm most likely dead then so I don't care that much ;)

> +
> + writel((u32)time, ioaddr + SRTC_LPSAR);
> +
> + /* clear alarm interrupt status bit */
> + writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
> +
> + mxc_rtc_sync_lp_locked(ioaddr);
> + return 0;
> +}
> +
> +/* This function is the RTC interrupt service routine. */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> + struct platform_device *pdev = dev_id;
> + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
> + void __iomem *ioaddr = pdata->ioaddr;
> + unsigned long flags;
> + u32 events = 0;
> + u32 lp_status;
> + u32 lp_cr;
> +
> + spin_lock_irqsave(&pdata->lock, flags);
> + if (clk_prepare_enable(pdata->clk)) {
> + spin_unlock_irqrestore(&pdata->lock, flags);
> + return IRQ_NONE;
> + }

You are not allowed to do a clk_prepare under a spinlock. That was the
original reason to split enabling a clk into clk_prepare and clk_enable.
Everything that can block is done in clk_prepare and only non blocking
things are done in clk_enable.
If you want to enable/disable the clock on demand you can clk_prepare()
in probe and clk_enable when you actually need it.

> +
> + lp_status = readl(ioaddr + SRTC_LPSR);
> + lp_cr = readl(ioaddr + SRTC_LPCR);
> +
> + /* update irq data & counter */
> + if (lp_status & SRTC_LPSR_ALP) {
> + if (lp_cr & SRTC_LPCR_ALP)
> + events = (RTC_AF | RTC_IRQF);
> +
> + /* disable further lp alarm interrupts */
> + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> + }
> +
> + /* Update interrupt enables */
> + writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> + /* clear interrupt status */
> + writel(lp_status, ioaddr + SRTC_LPSR);
> +
> + mxc_rtc_sync_lp_locked(ioaddr);
> + rtc_update_irq(pdata->rtc, 1, events);
> + clk_disable_unprepare(pdata->clk);
> + spin_unlock_irqrestore(&pdata->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * Enable clk and aquire spinlock
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_lock(struct mxc_rtc_data *const pdata)
> +{
> + int ret;
> +
> + spin_lock_irq(&pdata->lock);
> + ret = clk_prepare_enable(pdata->clk);
> + if (ret) {
> + spin_unlock_irq(&pdata->lock);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata)
> +{
> + clk_disable_unprepare(pdata->clk);
> + spin_unlock_irq(&pdata->lock);
> + return 0;
> +}
> +
> +/*
> + * This function reads the current RTC time into tm in Gregorian date.
> + *
> + * @param tm contains the RTC time value upon return
> + *
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> + time_t now;
> + int ret = mxc_rtc_lock(pdata);
> +
> + if (ret)
> + return ret;
> +
> + now = readl(pdata->ioaddr + SRTC_LPSCMR);
> + rtc_time_to_tm(now, tm);
> + ret = rtc_valid_tm(tm);
> + mxc_rtc_unlock(pdata);

I don't think this needs to be locked.

> + return ret;
> +}
> +
> +/*
> + * This function sets the internal RTC time based on tm in Gregorian date.
> + *
> + * @param tm the time value to be set in the RTC
> + *
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> + time64_t time = rtc_tm_to_time64(tm);
> + int ret;
> +
> + if (time > U32_MAX) {
> + dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX);
> + return -EINVAL;
> + }
> +
> + ret = mxc_rtc_lock(pdata);
> + if (ret)
> + return ret;
> +
> + writel(time, pdata->ioaddr + SRTC_LPSCMR);
> + mxc_rtc_sync_lp_locked(pdata->ioaddr);
> + return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * This function reads the current alarm value into the passed in \b alrm
> + * argument. It updates the \b alrm's pending field value based on the whether
> + * an alarm interrupt occurs or not.
> + *
> + * @param alrm contains the RTC alarm value upon return
> + *
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> + void __iomem *ioaddr = pdata->ioaddr;
> + int ret;
> +
> + ret = mxc_rtc_lock(pdata);
> + if (ret)
> + return ret;
> +
> + rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time);
> + alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP);
> + return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * Enable/Disable alarm interrupt
> + * The caller should hold the pdata->lock
> + */
> +static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata,
> + unsigned int enable)
> +{
> + u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR);
> +
> + if (enable)
> + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> + else
> + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +
> + writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
> +}
> +
> +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
> +{
> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> + int ret = mxc_rtc_lock(pdata);
> +
> + if (ret)
> + return ret;
> +
> + mxc_rtc_alarm_irq_enable_locked(pdata, enable);
> + return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * This function sets the RTC alarm based on passed in alrm.
> + *
> + * @param alrm the alarm value to be set in the RTC
> + *
> + * @return 0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> + int ret = mxc_rtc_lock(pdata);
> +
> + if (ret)
> + return ret;
> +
> + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);

Is it worth it to make this a separate function?

> + if (!ret) {
> + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
> + mxc_rtc_sync_lp_locked(pdata->ioaddr);
> + }
> + mxc_rtc_unlock(pdata);
> + return ret;
> +}
> +
> +static const struct rtc_class_ops mxc_rtc_ops = {
> + .read_time = mxc_rtc_read_time,
> + .set_time = mxc_rtc_set_time,
> + .read_alarm = mxc_rtc_read_alarm,
> + .set_alarm = mxc_rtc_set_alarm,
> + .alarm_irq_enable = mxc_rtc_alarm_irq_enable,
> +};
> +
> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
> +{
> + unsigned int timeout = REG_READ_TIMEOUT;
> +
> + while (!(readl(ioaddr) & flag)) {
> + if (!--timeout) {
> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);

Please use dev_* functions for printing. In this case the message should
probably be printed from the caller.

> + return -EBUSY;
> + }
> + }
> + return 0;
> +}
> +
> +static int mxc_rtc_probe(struct platform_device *pdev)
> +{
> + struct mxc_rtc_data *pdata;
> + struct resource *res;
> + void __iomem *ioaddr;
> + int ret = 0;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(pdata->ioaddr))
> + return PTR_ERR(pdata->ioaddr);
> +
> + ioaddr = pdata->ioaddr;
> +
> + pdata->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pdata->clk)) {
> + dev_err(&pdev->dev, "unable to get rtc clock!\n");
> + return PTR_ERR(pdata->clk);
> + }
> +
> + spin_lock_init(&pdata->lock);
> + pdata->irq = platform_get_irq(pdev, 0);
> + if (pdata->irq < 0)
> + return pdata->irq;
> +
> + device_init_wakeup(&pdev->dev, 1);
> +
> + ret = clk_prepare_enable(pdata->clk);
> + if (ret)
> + return ret;
> + /* initialize glitch detect */
> + writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
> +
> + /* clear lp interrupt status */
> + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +
> + /* move out of init state */
> + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
> + xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);

If this can fail, shouldn't you test for an error?

> +
> + /* move out of non-valid state */
> + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
> + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
> + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);

dito

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2017-12-06 08:58:40

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote:
> > +/*
> > + * This function updates the RTC alarm registers and then clears all the
> > + * interrupt status bits.
> > + * The caller should hold the pdata->lock
> > + *
> > + * @param alrm the new alarm value to be updated in the RTC
> > + *
> > + * @return 0 if successful; non-zero otherwise.
> > + */
> > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
> > + struct rtc_time *alarm_tm)
> > +{
> > + void __iomem *const ioaddr = pdata->ioaddr;
> > + unsigned long time;
> > +
> > + rtc_tm_to_time(alarm_tm, &time);
> > +
> > + if (time > U32_MAX) {
> > + pr_err("Hopefully I am out of service by then :-(\n");
> > + return -EINVAL;
> > + }
>
> This will never happen as on your target hardware unsigned long is a
> 32bit type. Not sure what is best to do here. Maybe you should test
> the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
> but rtc_tm_to_time could detect when the input time doesn't fit into
> its return type and return an error in this case.
> Also I just realized that it's unsigned and only overflows in the year
> 2106. I'm most likely dead then so I don't care that much ;)
>

One solution is to use the 64bit version instead so it doesn't overflow.
This makes the time > U32_MAX work.
Also, I'll send (hopefully soon) a series adding proper range checking
for the whole RTC subsystem. And yes, it not urgent as I don't think I
will care so much in 2106 too ;)

> > +/*
> > + * This function reads the current RTC time into tm in Gregorian date.
> > + *
> > + * @param tm contains the RTC time value upon return
> > + *
> > + * @return 0 if successful; non-zero otherwise.
> > + */
> > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> > + time_t now;
> > + int ret = mxc_rtc_lock(pdata);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + now = readl(pdata->ioaddr + SRTC_LPSCMR);
> > + rtc_time_to_tm(now, tm);
> > + ret = rtc_valid_tm(tm);

This check is useless for two reasons: you know that rtc_time_to_tm will
generate a valid tm and the core always checks the tm anyway.

--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2017-12-06 09:28:21

[permalink] [raw]

Subject: RE: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

>From: Alexandre Belloni [mailto:[email protected]]
>Sent: Mittwoch, 6. Dezember 2017 09:58
>On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote:
>> > +/*
>> > + * This function updates the RTC alarm registers and then clears all the
>> > + * interrupt status bits.
>> > + * The caller should hold the pdata->lock
>> > + *
>> > + * @param alrm the new alarm value to be updated in the RTC
>> > + *
>> > + * @return 0 if successful; non-zero otherwise.
>> > + */
>> > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
>> > + struct rtc_time *alarm_tm)
>> > +{
>> > + void __iomem *const ioaddr = pdata->ioaddr;
>> > + unsigned long time;
>> > +
>> > + rtc_tm_to_time(alarm_tm, &time);
>> > +
>> > + if (time > U32_MAX) {
>> > + pr_err("Hopefully I am out of service by then :-(\n");
>> > + return -EINVAL;
>> > + }
>>
>> This will never happen as on your target hardware unsigned long is a
>> 32bit type. Not sure what is best to do here. Maybe you should test
>> the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
>> but rtc_tm_to_time could detect when the input time doesn't fit into
>> its return type and return an error in this case.
>> Also I just realized that it's unsigned and only overflows in the year
>> 2106. I'm most likely dead then so I don't care that much ;)
>>
>
>One solution is to use the 64bit version instead so it doesn't overflow.
>This makes the time > U32_MAX work.
>Also, I'll send (hopefully soon) a series adding proper range checking
>for the whole RTC subsystem. And yes, it not urgent as I don't think I
>will care so much in 2106 too ;)
>
I just noticed that in mxc_rtc_set_time() I am using the 64bit version.
After thinking a while about this issue, I think the 64bit version is better
suited for my use case. It makes explicitly clear that I need to push the
time into a 32bit hw register and "unsigned long" is just by accident the
correct size for me.

>> > +/*
>> > + * This function reads the current RTC time into tm in Gregorian date.
>> > + *
>> > + * @param tm contains the RTC time value upon return
>> > + *
>> > + * @return 0 if successful; non-zero otherwise.
>> > + */
>> > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> > +{
>> > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> > + time_t now;
>> > + int ret = mxc_rtc_lock(pdata);
>> > +
>> > + if (ret)
>> > + return ret;
>> > +
>> > + now = readl(pdata->ioaddr + SRTC_LPSCMR);
>> > + rtc_time_to_tm(now, tm);
>> > + ret = rtc_valid_tm(tm);
>
>This check is useless for two reasons: you know that rtc_time_to_tm will
>generate a valid tm and the core always checks the tm anyway.
I will remove this with the next version

Thanks for your time and help,
Patrick

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

2017-12-06 10:17:15

[permalink] [raw]

Subject: RE: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

>From: Sascha Hauer [mailto:[email protected]]
>Sent: Mittwoch, 6. Dezember 2017 09:36
>On Tue, Dec 05, 2017 at 03:06:46PM +0100, [email protected]
>wrote:
>> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
>> + struct rtc_time *alarm_tm)
>> +{
>> + void __iomem *const ioaddr = pdata->ioaddr;
>> + unsigned long time;
>> +
>> + rtc_tm_to_time(alarm_tm, &time);
>> +
>> + if (time > U32_MAX) {
>> + pr_err("Hopefully I am out of service by then :-(\n");
>> + return -EINVAL;
>> + }
>
>This will never happen as on your target hardware unsigned long is a
>32bit type. Not sure what is best to do here. Maybe you should test
>the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
>but rtc_tm_to_time could detect when the input time doesn't fit into
>its return type and return an error in this case.
>Also I just realized that it's unsigned and only overflows in the year
>2106. I'm most likely dead then so I don't care that much ;)
>
please see my response to Alexandre's follow up

>> +/* This function is the RTC interrupt service routine. */
>> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
>> +{
>> + struct platform_device *pdev = dev_id;
>> + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
>> + void __iomem *ioaddr = pdata->ioaddr;
>> + unsigned long flags;
>> + u32 events = 0;
>> + u32 lp_status;
>> + u32 lp_cr;
>> +
>> + spin_lock_irqsave(&pdata->lock, flags);
>> + if (clk_prepare_enable(pdata->clk)) {
>> + spin_unlock_irqrestore(&pdata->lock, flags);
>> + return IRQ_NONE;
>> + }
>
>You are not allowed to do a clk_prepare under a spinlock. That was the
>original reason to split enabling a clk into clk_prepare and clk_enable.
>Everything that can block is done in clk_prepare and only non blocking
>things are done in clk_enable.
>If you want to enable/disable the clock on demand you can clk_prepare()
>in probe and clk_enable when you actually need it.
>
Thanks for clarification. To be honest when I read Lothar's suggestion it was
the first time I thought about the idea of keeping the clk disabled most of
the time. I am not very experienced with this. But a rtctest loop run for
hours so I assume it would be okay to keep the clk disabled until hw access.
If there is no objection from somebody who knows the i.MX53 SRTC HW
better, I will stick to the clock on demand model and make sure I avoid
blocking.
>> +
>> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> + time_t now;
>> + int ret = mxc_rtc_lock(pdata);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + now = readl(pdata->ioaddr + SRTC_LPSCMR);
>> + rtc_time_to_tm(now, tm);
>> + ret = rtc_valid_tm(tm);
>> + mxc_rtc_unlock(pdata);
>
>I don't think this needs to be locked.
I will change this to only enable the clock for the readl()

>> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> + int ret = mxc_rtc_lock(pdata);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);
>
>Is it worth it to make this a separate function?
Maybe not, I think it is an artifact from a refactoring. I will reconsider this
for the next version.
>
>> + if (!ret) {
>> + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
>> + mxc_rtc_sync_lp_locked(pdata->ioaddr);
>> + }
>> + mxc_rtc_unlock(pdata);
>> + return ret;
>> +}
>> +
>> +static const struct rtc_class_ops mxc_rtc_ops = {
>> + .read_time = mxc_rtc_read_time,
>> + .set_time = mxc_rtc_set_time,
>> + .read_alarm = mxc_rtc_read_alarm,
>> + .set_alarm = mxc_rtc_set_alarm,
>> + .alarm_irq_enable = mxc_rtc_alarm_irq_enable,
>> +};
>> +
>> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
>> +{
>> + unsigned int timeout = REG_READ_TIMEOUT;
>> +
>> + while (!(readl(ioaddr) & flag)) {
>> + if (!--timeout) {
>> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
>
>Please use dev_* functions for printing. In this case the message should
>probably be printed from the caller.
Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here,
because I would have to change the functions signature to get a device.
However, I will drop the message and move it to the caller.

>> + /* clear lp interrupt status */
>> + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
>> +
>> + /* move out of init state */
>> + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
>> + xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);
>
>If this can fail, shouldn't you test for an error?
very probably
>> +
>> + /* move out of non-valid state */
>> + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
>> + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
>> + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);
>
>dito
sure

Thanks,
Patrick
---
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

2017-12-06 11:05:08

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

On 05/12/2017 at 15:06:46 +0100, [email protected] wrote:
> +/* This function is the RTC interrupt service routine. */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> + struct platform_device *pdev = dev_id;
> + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
> + void __iomem *ioaddr = pdata->ioaddr;
> + unsigned long flags;
> + u32 events = 0;
> + u32 lp_status;
> + u32 lp_cr;
> +
> + spin_lock_irqsave(&pdata->lock, flags);
> + if (clk_prepare_enable(pdata->clk)) {
> + spin_unlock_irqrestore(&pdata->lock, flags);
> + return IRQ_NONE;
> + }
> +
> + lp_status = readl(ioaddr + SRTC_LPSR);
> + lp_cr = readl(ioaddr + SRTC_LPCR);
> +
> + /* update irq data & counter */
> + if (lp_status & SRTC_LPSR_ALP) {
> + if (lp_cr & SRTC_LPCR_ALP)
> + events = (RTC_AF | RTC_IRQF);

I would just call rtc_update_irq here...

> +
> + /* disable further lp alarm interrupts */
> + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> + }
> +
> + /* Update interrupt enables */
> + writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> + /* clear interrupt status */
> + writel(lp_status, ioaddr + SRTC_LPSR);
> +
> + mxc_rtc_sync_lp_locked(ioaddr);
> + rtc_update_irq(pdata->rtc, 1, events);

... because calling it here with events == 0 will result in a lot of
work for nothing (the core will walk through the timers and set the
alarm again).

--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2017-12-06 14:40:49

by Sascha Hauer

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

On Wed, Dec 06, 2017 at 10:17:06AM +0000, Patrick Br?nn wrote:
> >From: Sascha Hauer [mailto:[email protected]]
> >Sent: Mittwoch, 6. Dezember 2017 09:36
> >On Tue, Dec 05, 2017 at 03:06:46PM +0100, [email protected]
> >wrote:
> >> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
> >> +{
> >> + unsigned int timeout = REG_READ_TIMEOUT;
> >> +
> >> + while (!(readl(ioaddr) & flag)) {
> >> + if (!--timeout) {
> >> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
> >
> >Please use dev_* functions for printing. In this case the message should
> >probably be printed from the caller.
> Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here,
> because I would have to change the functions signature to get a device.
> However, I will drop the message and move it to the caller.

No, I don't have a link. However, a message printed with pr_err comes
with absolutely no context, so without grepping the source you do not
get a clue which device has a problem. You could add more context
using some prefix either to each message or by using

#define pr_fmt(fmt) "mydriver: " fmt

but then you still not know which instance the message throws. Using
dev_* just does the right thing without much thinking.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2017-12-06 21:54:41

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

On Tue, Dec 05, 2017 at 03:06:42PM +0100, [email protected] wrote:
> From: Patrick Bruenn <[email protected]>
>
> Document the binding for i.MX53 SRTC implemented by rtc-mxc_v2
>
> Signed-off-by: Patrick Bruenn <[email protected]><Paste>
>
> ---
>
> To: Alessandro Zummo <[email protected]>
> To: Alexandre Belloni <[email protected]>
>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: [email protected] (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: [email protected] (open list)
> Cc: Fabio Estevam <[email protected]>
> Cc: Juergen Borleis <[email protected]>
> Cc: Noel Vellemans <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <[email protected]> (maintainer:ARM PORT)
> Cc: [email protected] (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
>
> Cc: Philippe Ombredanne <[email protected]>
> Cc: Lothar Wa?mann <[email protected]>
> ---
> Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
>
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
> new file mode 100644
> index 000000000000..796e7f4995db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
> @@ -0,0 +1,17 @@
> +* i.MX53 Real Time Clock controller
> +
> +Required properties:
> +- compatible: should be: "fsl,imx53-rtc"
> +- reg: physical base address of the controller and length of memory mapped
> + region.
> +- clocks: should contain the phandle for the rtc clock

Shouldn't there be more than 1 here. From what I remember, the ipg clock
and the 32k clock?

> +- interrupts: rtc alarm interrupt
> +
> +Example:
> +
> +srtc@53fa4000 {

rtc@...

> + compatible = "fsl,imx53-rtc";
> + reg = <0x53fa4000 0x4000>;
> + interrupts = <24>;
> + clocks = <&clks IMX5_CLK_SRTC_GATE>;
> +};
> --
> 2.11.0

2017-12-10 19:03:23

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

On Tue, Dec 5, 2017 at 12:20 PM, Patrick Brünn <[email protected]> wrote:

> I will wait a few days for more reviewers and then integrate your comments in a v3. If nothing major show up I will wait until the imx53.dtsi revert landed in Linus tree.

It is in Linus' tree now as commit e501506d3ea0 ("Revert "ARM: dts:
imx53: add srtc node"").

2017-12-11 07:08:37

[permalink] [raw]

Subject: RE: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

>From: Rob Herring [mailto:[email protected]]
>Sent: Mittwoch, 6. Dezember 2017 22:55
>On Tue, Dec 05, 2017 at 03:06:42PM +0100, [email protected]
>wrote:
>> From: Patrick Bruenn <[email protected]>
>>
>> +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
>> @@ -0,0 +1,17 @@
>> +* i.MX53 Real Time Clock controller
>> +
>> +Required properties:
>> +- compatible: should be: "fsl,imx53-rtc"
>> +- reg: physical base address of the controller and length of memory
>mapped
>> + region.
>> +- clocks: should contain the phandle for the rtc clock
>
>Shouldn't there be more than 1 here. From what I remember, the ipg clock
>and the 32k clock?
Yes, you are right. But if I am reading the original Freescale driver and the
reference manual correctly, the second clock is always active.
Section "72.3.1.1 Clocks" from the reference manual [1] reads:
"SRTC runs on two clock sources, high frequency peripherals clock and low frequency
timers clock. The high frequency peripheral IP clock is used by the SRTC peripheral bus
interface, control and status registers. The low frequency 32.768 kHz clock is the
always-active clock used by the SRTC timers..."

That's why I would only include one clock . Should I change this?

>
>> +- interrupts: rtc alarm interrupt
>> +
>> +Example:
>> +
>> +srtc@53fa4000 {
>
>rtc@...
>
The rtc for which this series adds support is embedded within a function block called
"Secure Real Time Clock". This driver doesn't utilize all of the hardware features by
now. But maybe someone else wants to extend the functionalities, later.
For that possibility I wanted to name the node "srtc". Should I still change this?

I believe you have a much better understanding of what should be done here. I don't
want to argue with you, just thought you might not had that information. So if I am
wrong just tell me and I will change it without further "complaining".

Thank you for taking the time,
Patrick

[1] https://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf

---
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

2017-12-11 23:08:06

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

Hi Patrick,

On Mon, Dec 11, 2017 at 5:08 AM, Patrick Brünn <[email protected]> wrote:

>>rtc@...
>>
> The rtc for which this series adds support is embedded within a function block called
> "Secure Real Time Clock". This driver doesn't utilize all of the hardware features by
> now. But maybe someone else wants to extend the functionalities, later.
> For that possibility I wanted to name the node "srtc". Should I still change this?
>
> I believe you have a much better understanding of what should be done here. I don't
> want to argue with you, just thought you might not had that information. So if I am
> wrong just tell me and I will change it without further "complaining".

>From the Devicetree Specification document:

"Generic Names Recommendation

The name of a node should be somewhat generic, reflecting the function
of the device and not its precise program-
ming model. If appropriate, the name should be one of the following choices:
...
rtc
"

So better use 'rtc' as suggested by Rob.

2017-12-12 05:05:19