add mxc driver 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 linux-rtc at vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)

Cc devicetree at vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)

Cc linux-kernel at vger.kernel.org (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 linux-arm-kernel at lists.infradead.org (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

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +0 -0

new file mode 100644
index 000000000000..796e7f4995db

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +5 -1

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

2.11.0

From: Rob Herring [mailto:robh at kernel.org]
Sent: Mittwoch, 6. Dezember 2017 22:55

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 5

Quoted 9 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

+++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +5 -0

@@ -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

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 7

Quoted 2 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +2 -0

+  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?

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 12

Quoted 6 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +5 -0

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

rtc at ...

Quoted 2 lines

rtc at ...

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:

From: Fabio Estevam [mailto:festevam at gmail.com]
Sent: Dienstag, 12. Dezember 2017 00:08
Hi Patrick,

Quoted 3 lines · Patrick Br?nn

Quoted 2 lines

rtc at ...

The rtc for which this series adds support is embedded within a function

block called

Quoted 1 line

"Secure Real Time Clock". This driver doesn't utilize all of the hardware

features by

Quoted 2 lines

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?

Quoted 2 lines

I believe you have a much better understanding of what should be done

here. I don't

Quoted 1 line

want to argue with you, just thought you might not had that information. So

if I am

Quoted 1 line

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.

Quoted 14 lines

From: Rob Herring [mailto:robh at kernel.org]
Sent: Mittwoch, 6. Dezember 2017 22:55

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 5

Quoted 9 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

+++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +5 -0

@@ -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

Quoted 5 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 7

Quoted 2 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +2 -0

+  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?

Quoted 8 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt line 12

Quoted 5 lines

Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file +5 -0

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

rtc at ...

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: Fabio Estevam [mailto:festevam at gmail.com]
Sent: Dienstag, 5. Dezember 2017 15:14

arch/arm/boot/dts/imx53.dtsi line 442

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

                        srtc: srtc at 53fa4000 {

arch/arm/boot/dts/imx53.dtsi +1 -3

-                               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.

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.

From: Patrick Bruenn <[email protected]>

Add RTC_DRV_MXC_V2 config option

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

 arch/arm/configs/imx_v4_v5_defconfig | 1 +

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: linux-rtc at vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: devicetree at vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-kernel at vger.kernel.org (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: linux-arm-kernel at lists.infradead.org (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

drivers/rtc/Makefile +1 -0

index f2f50c11dc38..dcf60e61ae5c 100644
@@ -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

drivers/rtc/rtc-mxc_v2.c new file +79 -0

new file mode 100644
index 000000000000..c5a6d2c293bb
@@ -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)

drivers/rtc/rtc-mxc_v2.c new file +16 -0

+
+	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;

drivers/rtc/rtc-mxc_v2.c new file +60 -0

+
+	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);

drivers/rtc/rtc-mxc_v2.c new file +89 -0

+	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.

drivers/rtc/rtc-mxc_v2.c new file +13 -0

+	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,

drivers/rtc/rtc-mxc_v2.c new file +42 -0

+			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)

Quoted 11 lines

drivers/rtc/rtc-mxc_v2.c new file +11 -0

+/*
+ * 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)

drivers/rtc/rtc-mxc_v2.c line 89

Quoted 10 lines

drivers/rtc/rtc-mxc_v2.c new file +10 -0

+{
+	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 ;)

Quoted 11 lines

drivers/rtc/rtc-mxc_v2.c new file +11 -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;

drivers/rtc/rtc-mxc_v2.c line 184

Quoted 8 lines

drivers/rtc/rtc-mxc_v2.c new file +8 -0

+	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);

From: Alexandre Belloni [mailto:alexandre.belloni at free-electrons.com]
Sent: Mittwoch, 6. Dezember 2017 09:58

Quoted 22 lines · Sascha Hauer

Quoted 11 lines

drivers/rtc/rtc-mxc_v2.c new file +11 -0

+/*
+ * 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)

drivers/rtc/rtc-mxc_v2.c line 89

Quoted 10 lines

drivers/rtc/rtc-mxc_v2.c new file +10 -0

+{
+  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;
+  }

Quoted 8 lines

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 ;)

Quoted 14 lines

Quoted 14 lines

drivers/rtc/rtc-mxc_v2.c new file +14 -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)

Quoted 5 lines

drivers/rtc/rtc-mxc_v2.c line 184

Quoted 5 lines

drivers/rtc/rtc-mxc_v2.c new file +5 -0

+          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.

From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Mittwoch, 6. Dezember 2017 09:36

drivers/rtc/rtc-mxc_v2.c line 89

Quoted 12 lines · linux-kernel-dev at beckhoff.com

drivers/rtc/rtc-mxc_v2.c new file +12 -0

+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 ;)

Quoted 15 lines

drivers/rtc/rtc-mxc_v2.c new file +15 -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;

Quoted 1 line

+ }

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.

Quoted 8 lines

drivers/rtc/rtc-mxc_v2.c new file +8 -0

+
+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)

drivers/rtc/rtc-mxc_v2.c line 185

Quoted 6 lines

drivers/rtc/rtc-mxc_v2.c new file +6 -0

+            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.

drivers/rtc/rtc-mxc_v2.c line 284

Quoted 9 lines

drivers/rtc/rtc-mxc_v2.c new file +9 -0

+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?

Quoted 19 lines

drivers/rtc/rtc-mxc_v2.c new file +18 -0

+    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)
+{

drivers/rtc/rtc-mxc_v2.c line 307

Quoted 5 lines

drivers/rtc/rtc-mxc_v2.c new file +5 -0

+    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.

drivers/rtc/rtc-mxc_v2.c

Quoted 6 lines

drivers/rtc/rtc-mxc_v2.c new file +6 -0

+    /* 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?

drivers/rtc/rtc-mxc_v2.c line 364

Quoted 5 lines

drivers/rtc/rtc-mxc_v2.c new file +5 -0

+
+    /* 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

Quoted 4 lines

From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Mittwoch, 6. Dezember 2017 09:36
On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev at beckhoff.com wrote:

Quoted 10 lines

drivers/rtc/rtc-mxc_v2.c line 307

Quoted 7 lines

drivers/rtc/rtc-mxc_v2.c new file +7 -0

+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.

drivers/rtc/rtc-mxc_v2.c new file +14 -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);