[PATCH v2] rtc: snvs: add Freescale rtc-snvs driver

Add an RTC driver for Freescale Secure Non-Volatile Storage (SNVS)
Low Power (LP) RTC.

Signed-off-by: Shawn Guo <redacted>
Cc: Alessandro Zummo <redacted>
---
Changes since v1:
* Address the comments Stephen and Kim gave on v1

 .../devicetree/bindings/crypto/fsl-sec4.txt        |   51 +++
 Documentation/devicetree/bindings/rtc/snvs-rtc.txt |    1 +
 drivers/rtc/Kconfig                                |   11 +
 drivers/rtc/Makefile                               |    1 +
 drivers/rtc/rtc-snvs.c                             |  355 ++++++++++++++++++++
 5 files changed, 419 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/snvs-rtc.txt
 create mode 100644 drivers/rtc/rtc-snvs.c

diff --git a/Documentation/devicetree/bindings/crypto/fsl-sec4.txt b/Documentation/devicetree/bindings/crypto/fsl-sec4.txt
index bf57ecd..bd7ce12 100644
--- a/Documentation/devicetree/bindings/crypto/fsl-sec4.txt
+++ b/Documentation/devicetree/bindings/crypto/fsl-sec4.txt

@@ -9,6 +9,7 @@ Copyright (C) 2008-2011 Freescale Semiconductor Inc.
    -Run Time Integrity Check (RTIC) Node
    -Run Time Integrity Check (RTIC) Memory Node
    -Secure Non-Volatile Storage (SNVS) Node
+   -Secure Non-Volatile Storage (SNVS) Low Power (LP) RTC Node
    -Full Example
 
 NOTE: the SEC 4 is also known as Freescale's Cryptographic Accelerator

@@ -294,6 +295,27 @@ Secure Non-Volatile Storage (SNVS) Node
           address and length of the SEC4 configuration
           registers.
 
+   - #address-cells
+       Usage: required
+       Value type: <u32>
+       Definition: A standard property.  Defines the number of cells
+           for representing physical addresses in child nodes.  Must
+           have a value of 1.
+
+   - #size-cells
+       Usage: required
+       Value type: <u32>
+       Definition: A standard property.  Defines the number of cells
+           for representing the size of physical addresses in
+           child nodes.  Must have a value of 1.
+
+   - ranges
+       Usage: required
+       Value type: <prop-encoded-array>
+       Definition: A standard property.  Specifies the physical address
+           range of the SNVS register space.  A triplet that includes
+           the child address, parent address, & length.
+
    - interrupts
       Usage: required
       Value type: <prop_encoded-array>

@@ -314,11 +336,34 @@ EXAMPLE
 	sec_mon at 314000 {
 		compatible = "fsl,sec-v4.0-mon";
 		reg = <0x314000 0x1000>;
+		ranges = <0 0x314000 0x1000>;
 		interrupt-parent = <&mpic>;
 		interrupts = <93 2>;
 	};
 
 =====================================================================
+Secure Non-Volatile Storage (SNVS) Low Power (LP) RTC Node
+
+  A SNVS child node that defines SNVS LP RTC.
+
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: Must include "fsl,sec-v4.0-mon-rtc-lp".
+
+  - reg
+      Usage: required
+      Value type: <prop-encoded-array>
+      Definition: A standard property.  Specifies the physical
+          address and length of the SNVS LP configuration registers.
+
+EXAMPLE
+	sec_mon_rtc_lp at 314000 {
+		compatible = "fsl,sec-v4.0-mon-rtc-lp";
+		reg = <0x34 0x58>;
+	};
+
+=====================================================================
 FULL EXAMPLE
 
 	crypto: crypto at 300000 {

@@ -390,8 +435,14 @@ FULL EXAMPLE
 	sec_mon: sec_mon at 314000 {
 		compatible = "fsl,sec-v4.0-mon";
 		reg = <0x314000 0x1000>;
+		ranges = <0 0x314000 0x1000>;
 		interrupt-parent = <&mpic>;
 		interrupts = <93 2>;
+
+		sec_mon_rtc_lp at 34 {
+			compatible = "fsl,sec-v4.0-mon-rtc-lp";
+			reg = <0x34 0x58>;
+		};
 	};
 
 =====================================================================

diff --git a/Documentation/devicetree/bindings/rtc/snvs-rtc.txt b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt
new file mode 100644
index 0000000..fb61ed7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt

@@ -0,0 +1 @@
+See Documentation/devicetree/bindings/crypto/fsl-sec4.txt for details.

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 08cbdb9..e174e16 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig

@@ -1087,4 +1087,15 @@ 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_SNVS
+	tristate "Freescale SNVS RTC support"
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	   If you say yes here you get support for the Freescale SNVS
+	   Low Power (LP) RTC module.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-snvs".
+
 endif # RTC_CLASS

diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 2973921..0e3ff92 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile

@@ -95,6 +95,7 @@ obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
 obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
 obj-$(CONFIG_RTC_DRV_SA1100)	+= rtc-sa1100.o
 obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
+obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.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-snvs.c b/drivers/rtc/rtc-snvs.c
new file mode 100644
index 0000000..7c034e0
--- /dev/null
+++ b/drivers/rtc/rtc-snvs.c

@@ -0,0 +1,355 @@
+/*
+ * Copyright (C) 2011-2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+/* These register offsets are relative to LP (Low Power) range */
+#define SNVS_LPCR		0x04
+#define SNVS_LPSR		0x18
+#define SNVS_LPSRTCMR		0x1c
+#define SNVS_LPSRTCLR		0x20
+#define SNVS_LPTAR		0x24
+#define SNVS_LPPGDR		0x30
+
+#define SNVS_LPCR_SRTC_ENV	(1 << 0)
+#define SNVS_LPCR_LPTA_EN	(1 << 1)
+#define SNVS_LPCR_LPWUI_EN	(1 << 3)
+#define SNVS_LPSR_LPTA		(1 << 0)
+
+#define SNVS_LPPGDR_INIT	0x41736166
+#define CNTR_TO_SECS_SH		15
+
+struct snvs_rtc_data {
+	struct rtc_device *rtc;
+	void __iomem *ioaddr;
+	int irq;
+	spinlock_t lock;
+};
+
+static u32 rtc_read_lp_counter(void __iomem *ioaddr)
+{
+	u64 read1, read2;
+
+	do {
+		read1 = readl(ioaddr + SNVS_LPSRTCMR);
+		read1 <<= 32;
+		read1 |= readl(ioaddr + SNVS_LPSRTCLR);
+
+		read2 = readl(ioaddr + SNVS_LPSRTCMR);
+		read2 <<= 32;
+		read2 |= readl(ioaddr + SNVS_LPSRTCLR);
+	} while (read1 != read2);
+
+	/* Convert 47-bit counter to 32-bit raw second count */
+	return (u32) (read1 >> CNTR_TO_SECS_SH);
+}
+
+static void rtc_write_sync_lp(void __iomem *ioaddr)
+{
+	u32 count1, count2, count3;
+	int i;
+
+	/* Wait for 3 CKIL cycles */
+	for (i = 0; i < 3; i++) {
+		do {
+			count1 = readl(ioaddr + SNVS_LPSRTCLR);
+			count2 = readl(ioaddr + SNVS_LPSRTCLR);
+		} while (count1 != count2);
+
+		/* Now wait until counter value changes */
+		do {
+			do {
+				count2 = readl(ioaddr + SNVS_LPSRTCLR);
+				count3 = readl(ioaddr + SNVS_LPSRTCLR);
+			} while (count2 != count3);
+		} while (count3 == count1);
+	}
+}
+
+static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(1);
+	unsigned long flags;
+	u32 lpcr;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	lpcr = readl(data->ioaddr + SNVS_LPCR);
+	if (enable)
+		lpcr |= SNVS_LPCR_SRTC_ENV;
+	else
+		lpcr &= ~SNVS_LPCR_SRTC_ENV;
+	writel(lpcr, data->ioaddr + SNVS_LPCR);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	while (1) {
+		lpcr = readl(data->ioaddr + SNVS_LPCR);
+
+		if (enable) {
+			if (lpcr & SNVS_LPCR_SRTC_ENV)
+				break;
+		} else {
+			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+				break;
+		}
+
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int snvs_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	unsigned long time = rtc_read_lp_counter(data->ioaddr);
+
+	rtc_time_to_tm(time, tm);
+
+	return 0;
+}
+
+static int snvs_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	unsigned long time;
+
+	rtc_tm_to_time(tm, &time);
+
+	/* Disable RTC first */
+	snvs_rtc_enable(data, false);
+
+	/* Write 32-bit time to 47-bit timer, leaving 15 LSBs blank */
+	writel(time << CNTR_TO_SECS_SH, data->ioaddr + SNVS_LPSRTCLR);
+	writel(time >> (32 - CNTR_TO_SECS_SH), data->ioaddr + SNVS_LPSRTCMR);
+
+	/* Enable RTC again */
+	snvs_rtc_enable(data, true);
+
+	return 0;
+}
+
+static int snvs_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	u32 lptar, lpsr;
+
+	lptar = readl(data->ioaddr + SNVS_LPTAR);
+	rtc_time_to_tm(lptar, &alrm->time);
+
+	lpsr = readl(data->ioaddr + SNVS_LPSR);
+	alrm->pending = (lpsr & SNVS_LPSR_LPTA) ? 1 : 0;
+
+	return 0;
+}
+
+static int snvs_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	u32 lpcr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	lpcr = readl(data->ioaddr + SNVS_LPCR);
+	if (enable)
+		lpcr |= (SNVS_LPCR_LPTA_EN | SNVS_LPCR_LPWUI_EN);
+	else
+		lpcr &= ~(SNVS_LPCR_LPTA_EN | SNVS_LPCR_LPWUI_EN);
+	writel(lpcr, data->ioaddr + SNVS_LPCR);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	rtc_write_sync_lp(data->ioaddr);
+
+	return 0;
+}
+
+static int snvs_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	struct rtc_time *alrm_tm = &alrm->time;
+	unsigned long time;
+	unsigned long flags;
+	u32 lpcr;
+
+	rtc_tm_to_time(alrm_tm, &time);
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	/* Have to clear LPTA_EN before programming new alarm time in LPTAR */
+	lpcr = readl(data->ioaddr + SNVS_LPCR);
+	lpcr &= ~SNVS_LPCR_LPTA_EN;
+	writel(lpcr, data->ioaddr + SNVS_LPCR);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	writel(time, data->ioaddr + SNVS_LPTAR);
+
+	/* Clear alarm interrupt status bit */
+	writel(SNVS_LPSR_LPTA, data->ioaddr + SNVS_LPSR);
+
+	return snvs_rtc_alarm_irq_enable(dev, alrm->enabled);
+}
+
+static const struct rtc_class_ops snvs_rtc_ops = {
+	.read_time = snvs_rtc_read_time,
+	.set_time = snvs_rtc_set_time,
+	.read_alarm = snvs_rtc_read_alarm,
+	.set_alarm = snvs_rtc_set_alarm,
+	.alarm_irq_enable = snvs_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t snvs_rtc_irq_handler(int irq, void *dev_id)
+{
+	struct device *dev = dev_id;
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	u32 lpsr;
+	u32 events = 0;
+
+	lpsr = readl(data->ioaddr + SNVS_LPSR);
+
+	if (lpsr & SNVS_LPSR_LPTA) {
+		events |= (RTC_AF | RTC_IRQF);
+
+		/* RTC alarm should be one-shot */
+		snvs_rtc_alarm_irq_enable(dev, 0);
+
+		rtc_update_irq(data->rtc, 1, events);
+	}
+
+	/* clear interrupt status */
+	writel(lpsr, data->ioaddr + SNVS_LPSR);
+
+	return events ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int __devinit snvs_rtc_probe(struct platform_device *pdev)
+{
+	struct snvs_rtc_data *data;
+	struct resource *res;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
+	if (!data->ioaddr)
+		return -EADDRNOTAVAIL;
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0)
+		return data->irq;
+
+	platform_set_drvdata(pdev, data);
+
+	spin_lock_init(&data->lock);
+
+	/* Initialize glitch detect */
+	writel(SNVS_LPPGDR_INIT, data->ioaddr + SNVS_LPPGDR);
+
+	/* Clear interrupt status */
+	writel(0xffffffff, data->ioaddr + SNVS_LPSR);
+
+	/* Enable RTC */
+	snvs_rtc_enable(data, true);
+
+	device_init_wakeup(&pdev->dev, true);
+
+	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
+			       IRQF_SHARED, "rtc alarm", &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+			data->irq, ret);
+		return ret;
+	}
+
+	data->rtc = rtc_device_register(pdev->name, &pdev->dev,
+					&snvs_rtc_ops, THIS_MODULE);
+	if (IS_ERR(data->rtc)) {
+		ret = PTR_ERR(data->rtc);
+		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit snvs_rtc_remove(struct platform_device *pdev)
+{
+	struct snvs_rtc_data *data = platform_get_drvdata(pdev);
+
+	rtc_device_unregister(data->rtc);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int snvs_rtc_suspend(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static int snvs_rtc_resume(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops snvs_rtc_pm_ops = {
+	.suspend = snvs_rtc_suspend,
+	.resume = snvs_rtc_resume,
+};
+#endif
+
+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, snvs_dt_ids);
+
+static struct platform_driver snvs_rtc_driver = {
+	.driver = {
+		.name	= "snvs_rtc",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &snvs_rtc_pm_ops,
+#endif
+		.of_match_table = snvs_dt_ids,
+	},
+	.probe		= snvs_rtc_probe,
+	.remove		= __devexit_p(snvs_rtc_remove),
+};
+module_platform_driver(snvs_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Freescale SNVS RTC Driver");
+MODULE_LICENSE("GPL");

-- 
1.7.5.4

On 07/04/12 08:38, Shawn Guo wrote:

Add an RTC driver for Freescale Secure Non-Volatile Storage (SNVS)
Low Power (LP) RTC.

Signed-off-by: Shawn Guo <redacted>
Cc: Alessandro Zummo <redacted>

Besides the minor comment below, please add

Reviewed-by: Stephen Boyd <redacted>

+#ifdef CONFIG_PM
+static int snvs_rtc_suspend(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static int snvs_rtc_resume(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops snvs_rtc_pm_ops = {
+	.suspend = snvs_rtc_suspend,
+	.resume = snvs_rtc_resume,
+};

Can you use the SIMPLE_DEV_PM_OPS? Then we get the same callbacks for
hibernation.

+#endif
+
+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, snvs_dt_ids);
+
+static struct platform_driver snvs_rtc_driver = {
+	.driver = {
+		.name	= "snvs_rtc",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &snvs_rtc_pm_ops,
+#endif

Doing that avoids having an ifdef here.

+		.of_match_table = snvs_dt_ids,
+	},
+	.probe		= snvs_rtc_probe,
+	.remove		= __devexit_p(snvs_rtc_remove),
+};
+module_platform_driver(snvs_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Freescale SNVS RTC Driver");
+MODULE_LICENSE("GPL");

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

On Wed, 4 Jul 2012 23:38:14 +0800
Shawn Guo [off-list ref] wrote:

+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },

the driver should be compatible with "fsl,sec-v4.0-mon", and its
probe function search for "fsl,sec-v4.0-mon-rtc-lp" sub-compatibles
(and possibly "fsl,sec-v4.0-mon-rtc-hp" in the future?).

Kim

On Thu, Jul 05, 2012 at 06:28:29PM -0500, Kim Phillips wrote:

On Wed, 4 Jul 2012 23:38:14 +0800
Shawn Guo [off-list ref] wrote:

quoted

+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },

the driver should be compatible with "fsl,sec-v4.0-mon", and its
probe function search for "fsl,sec-v4.0-mon-rtc-lp" sub-compatibles
(and possibly "fsl,sec-v4.0-mon-rtc-hp" in the future?).

So you think snvs is all about rtc (lp and hp), nothing else?  If that
is true, it's reasonable to do what you suggested here.  Otherwise, we
should have a snvs driver be compatible with "fsl,sec-v4.0-mon", and
this driver should be responsible for populate all the sub-devices of
snvs, rtc-lp, rtc-hp, and others.  In the latter case, rtc-snvs should
just stay as it stands right now.

-- 
Regards,
Shawn

On Fri, 6 Jul 2012 08:22:56 +0800
Shawn Guo [off-list ref] wrote:

On Thu, Jul 05, 2012 at 06:28:29PM -0500, Kim Phillips wrote:

quoted

On Wed, 4 Jul 2012 23:38:14 +0800
Shawn Guo [off-list ref] wrote:

quoted

+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },

the driver should be compatible with "fsl,sec-v4.0-mon", and its
probe function search for "fsl,sec-v4.0-mon-rtc-lp" sub-compatibles
(and possibly "fsl,sec-v4.0-mon-rtc-hp" in the future?).

So you think snvs is all about rtc (lp and hp), nothing else?  If that

no, I'm aware that it's more than a couple of RTCs.

is true, it's reasonable to do what you suggested here.  Otherwise, we
should have a snvs driver be compatible with "fsl,sec-v4.0-mon", and
this driver should be responsible for populate all the sub-devices of
snvs, rtc-lp, rtc-hp, and others.  In the latter case, rtc-snvs should
just stay as it stands right now.

but it doesn't function as it stands right now, at least on Power.
The compatible in the device tree's sec_mon node "fsl,sec-v4.0-mon"
and the driver's "fsl,sec-v4.0-mon-rtc-lp" don't match.  Here are
the device tree changes I used:

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi
index 7990e0d..14c933b 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi

@@ -104,6 +104,14 @@ crypto: crypto at 300000 {
 
 sec_mon: sec_mon at 314000 {
        compatible = "fsl,sec-v4.2-mon", "fsl,sec-v4.0-mon";
+       #address-cells = <1>;
+       #size-cells = <1>;
+       ranges = <0 0x314000 0x1000>;
        reg = <0x314000 0x1000>;
        interrupts = <93 2 0 0>;
+
+       sec_mon_rtc_lp at 34 {
+               compatible = "fsl,sec-v4.2-mon-rtc-lp", "fsl,sec-v4.0-mon-rtc-lp";
+               reg = <0x34 0x58>;
+       };
 };

btw, I don't see any imx6q.dtsi changes.

Kim

On Fri, Jul 06, 2012 at 05:56:34PM -0500, Kim Phillips wrote:

Yes, the way that this dts is written as well as the examples in
Documentation/devicetree/bindings/crypto/fsl-sec4.txt assume there
is a sec_mon driver.  This driver will match "fsl,sec-v4.0-mon" and
populate rtc-lp device probed by the rtc-snvs driver created by the
patch.

While there is no such sec_mon driver right now.  I did a quick
tweaking on imx6q.dtsi to have rtc-snvs probed without the need of
sec_mon driver.

btw, I don't see any imx6q.dtsi changes.

See below.

Either way, being a driver of SNVS LP RTC, rtc-snvs should just stand
as it stands right now.  It should not care about how the device is
populated, by DT core or by sec_mon driver.

Regards,
Shawn

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 8c90cba..26c42da 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi

@@ -455,8 +455,16 @@
                        };

                        snvs at 020cc000 {
-                               reg = <0x020cc000 0x4000>;
-                               interrupts = <0 19 0x04 0 20 0x04>;
+                               compatible = "fsl,sec-v4.0-mon", "simple-bus";
+                               #address-cells = <1>;
+                               #size-cells = <1>;
+                               ranges = <0 0x020cc000 0x4000>;
+
+                               snvs-rtc-lp at 34 {
+                                       compatible = "fsl,sec-v4.0-mon-rtc-lp";
+                                       reg = <0x34 0x58>;
+                                       interrupts = <0 19 0x04 0 20 0x04>;
+                               };
                        };

                        epit at 020d0000 { /* EPIT1 */

On Mon, 9 Jul 2012 14:51:18 +0800
Shawn Guo [off-list ref] wrote:

On Fri, Jul 06, 2012 at 05:56:34PM -0500, Kim Phillips wrote:

quoted

but it doesn't function as it stands right now, at least on Power.
The compatible in the device tree's sec_mon node "fsl,sec-v4.0-mon"
and the driver's "fsl,sec-v4.0-mon-rtc-lp" don't match.  Here are
the device tree changes I used:

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi
index 7990e0d..14c933b 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi

@@ -104,6 +104,14 @@ crypto: crypto at 300000 {
 
 sec_mon: sec_mon at 314000 {
        compatible = "fsl,sec-v4.2-mon", "fsl,sec-v4.0-mon";
+       #address-cells = <1>;
+       #size-cells = <1>;
+       ranges = <0 0x314000 0x1000>;
        reg = <0x314000 0x1000>;
        interrupts = <93 2 0 0>;
+
+       sec_mon_rtc_lp at 34 {
+               compatible = "fsl,sec-v4.2-mon-rtc-lp", "fsl,sec-v4.0-mon-rtc-lp";
+               reg = <0x34 0x58>;
+       };
 };

Yes, the way that this dts is written as well as the examples in
Documentation/devicetree/bindings/crypto/fsl-sec4.txt assume there
is a sec_mon driver.  This driver will match "fsl,sec-v4.0-mon" and
populate rtc-lp device probed by the rtc-snvs driver created by the
patch.

While there is no such sec_mon driver right now.  I did a quick
tweaking on imx6q.dtsi to have rtc-snvs probed without the need of
sec_mon driver.

...

ah, "simple-bus" was the missing tweak.

I'm not sure if this is appropriate vs. having common arch code
publish SNVS devices; see e.g., the other fsl,* devices in
arch/powerpc/platforms/85xx/common.c:mpc85xx_common_ids[].

Either way, I'd like to get support without having to manually tweak
the device tree, i.e., the patch doesn't work as-is.

btw, testing on a p4080r2 produces a soft lockup:

root at p4080ds:~# modprobe rtc-snvs
snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1
root at p4080ds:~# hwclock -w -f /dev/rtc1 
BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:0:4]
Modules linked in: rtc_snvs nfsd exportfs
irq event stamp: 54
hardirqs last  enabled at (53): [<c04f90dc>] _raw_spin_unlock_irq+0x30/0x58
hardirqs last disabled at (54): [<c04f8808>] _raw_spin_lock_irq+0x24/0x70
softirqs last  enabled at (0): [<c001eff8>] copy_process+0x2f4/0xde4
softirqs last disabled at (0): [<  (null)>]   (null)
NIP: f10b80b4 LR: f10b8074 CTR: 00000003
REGS: ee071d70 TRAP: 0901   Not tainted  (3.5.0-rc5-00099-gd8715a7-dirty)
MSR: 00029002 <CE,EE,ME>  CR: 42042022  XER: 00000000
TASK = ee06f470[4] 'kworker/0:0' THREAD: ee070000 CPU: 0
GPR00: 00000000 ee071e20 ee06f470 c04f9058 00000001 f10b8074 00000000 00000002 
GPR08: 00000001 f10bc054 00000000 00000000 c04f901c 
NIP [f10b80b4] snvs_rtc_alarm_irq_enable+0xb4/0xf8 [rtc_snvs]
LR [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs]
Call Trace:
[ee071e20] [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs] (unreliable)
[ee071e40] [c038c118] rtc_timer_do_work+0x184/0x20c
[ee071f10] [c003fc4c] process_one_work+0x1d4/0x48c
[ee071f50] [c0040d4c] worker_thread+0x184/0x358
[ee071f90] [c0046458] kthread+0x84/0x88
[ee071ff0] [c000d434] kernel_thread+0x4c/0x68
Instruction dump:
7c0004ac 7d404c2c 0c0a0000 4c00012c 7c0004ac 7c004c2c 0c000000 4c00012c 
7f8a0000 409effdc 7c0004ac 7c004c2c <0c000000> 4c00012c 7c0004ac 7d604c2c 

I debugged it to the innermost loop of rtc_write_sync_lp(), where
count2 and count3 are always equal.  Should there be a timeout in
that loop?  Do you know if this is because the machine isn't
in trusted mode?  Any other hints appreciated.

Kim

On Mon, Jul 09, 2012 at 07:45:14PM -0500, Kim Phillips wrote:

I'm not sure if this is appropriate vs. having common arch code
publish SNVS devices; see e.g., the other fsl,* devices in
arch/powerpc/platforms/85xx/common.c:mpc85xx_common_ids[].

This is completely beyond the scope of the patch, which merely adds
a driver for snvs-lp-rtc device.  But the driver does not know how
the device is created, and it does not need to know.

Either way, I'd like to get support without having to manually tweak
the device tree, i.e., the patch doesn't work as-is.

As I already said, the patch only adds a driver for snvs-lp-rtc.  It
should not care about how snvs-lp-rtc device is created on particular
platform.  The patch does work as-is for imx6q, which is the target
of the patch so far.

btw, testing on a p4080r2 produces a soft lockup:

root at p4080ds:~# modprobe rtc-snvs
snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1
root at p4080ds:~# hwclock -w -f /dev/rtc1 
BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:0:4]
Modules linked in: rtc_snvs nfsd exportfs
irq event stamp: 54
hardirqs last  enabled at (53): [<c04f90dc>] _raw_spin_unlock_irq+0x30/0x58
hardirqs last disabled at (54): [<c04f8808>] _raw_spin_lock_irq+0x24/0x70
softirqs last  enabled at (0): [<c001eff8>] copy_process+0x2f4/0xde4
softirqs last disabled at (0): [<  (null)>]   (null)
NIP: f10b80b4 LR: f10b8074 CTR: 00000003
REGS: ee071d70 TRAP: 0901   Not tainted  (3.5.0-rc5-00099-gd8715a7-dirty)
MSR: 00029002 <CE,EE,ME>  CR: 42042022  XER: 00000000
TASK = ee06f470[4] 'kworker/0:0' THREAD: ee070000 CPU: 0
GPR00: 00000000 ee071e20 ee06f470 c04f9058 00000001 f10b8074 00000000 00000002 
GPR08: 00000001 f10bc054 00000000 00000000 c04f901c 
NIP [f10b80b4] snvs_rtc_alarm_irq_enable+0xb4/0xf8 [rtc_snvs]
LR [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs]
Call Trace:
[ee071e20] [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs] (unreliable)
[ee071e40] [c038c118] rtc_timer_do_work+0x184/0x20c
[ee071f10] [c003fc4c] process_one_work+0x1d4/0x48c
[ee071f50] [c0040d4c] worker_thread+0x184/0x358
[ee071f90] [c0046458] kthread+0x84/0x88
[ee071ff0] [c000d434] kernel_thread+0x4c/0x68
Instruction dump:
7c0004ac 7d404c2c 0c0a0000 4c00012c 7c0004ac 7c004c2c 0c000000 4c00012c 
7f8a0000 409effdc 7c0004ac 7c004c2c <0c000000> 4c00012c 7c0004ac 7d604c2c 

I debugged it to the innermost loop of rtc_write_sync_lp(), where
count2 and count3 are always equal.  Should there be a timeout in
that loop?  Do you know if this is because the machine isn't
in trusted mode?  Any other hints appreciated.

This is probably because readl/writel does not work on powerpc.
I experienced the same thing when working on fsl_ssi driver.

My patch only targets imx6q right now.

-- 
Regards,
Shawn

On Tue, 10 Jul 2012 10:32:29 +0800
Shawn Guo [off-list ref] wrote:

On Mon, Jul 09, 2012 at 07:45:14PM -0500, Kim Phillips wrote:

quoted

I'm not sure if this is appropriate vs. having common arch code
publish SNVS devices; see e.g., the other fsl,* devices in
arch/powerpc/platforms/85xx/common.c:mpc85xx_common_ids[].

This is completely beyond the scope of the patch, which merely adds
a driver for snvs-lp-rtc device.  But the driver does not know how
the device is created, and it does not need to know.

quoted

Either way, I'd like to get support without having to manually tweak
the device tree, i.e., the patch doesn't work as-is.

As I already said, the patch only adds a driver for snvs-lp-rtc.  It
should not care about how snvs-lp-rtc device is created on particular
platform.  The patch does work as-is for imx6q, which is the target
of the patch so far.

how, without the "simple-bus" addition to the sec_mon compatibles
list in the imx6 device tree (which isn't present in this patch)?

quoted

btw, testing on a p4080r2 produces a soft lockup:

root at p4080ds:~# modprobe rtc-snvs
snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1
root at p4080ds:~# hwclock -w -f /dev/rtc1 
BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:0:4]
Modules linked in: rtc_snvs nfsd exportfs
irq event stamp: 54
hardirqs last  enabled at (53): [<c04f90dc>] _raw_spin_unlock_irq+0x30/0x58
hardirqs last disabled at (54): [<c04f8808>] _raw_spin_lock_irq+0x24/0x70
softirqs last  enabled at (0): [<c001eff8>] copy_process+0x2f4/0xde4
softirqs last disabled at (0): [<  (null)>]   (null)
NIP: f10b80b4 LR: f10b8074 CTR: 00000003
REGS: ee071d70 TRAP: 0901   Not tainted  (3.5.0-rc5-00099-gd8715a7-dirty)
MSR: 00029002 <CE,EE,ME>  CR: 42042022  XER: 00000000
TASK = ee06f470[4] 'kworker/0:0' THREAD: ee070000 CPU: 0
GPR00: 00000000 ee071e20 ee06f470 c04f9058 00000001 f10b8074 00000000 00000002 
GPR08: 00000001 f10bc054 00000000 00000000 c04f901c 
NIP [f10b80b4] snvs_rtc_alarm_irq_enable+0xb4/0xf8 [rtc_snvs]
LR [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs]
Call Trace:
[ee071e20] [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs] (unreliable)
[ee071e40] [c038c118] rtc_timer_do_work+0x184/0x20c
[ee071f10] [c003fc4c] process_one_work+0x1d4/0x48c
[ee071f50] [c0040d4c] worker_thread+0x184/0x358
[ee071f90] [c0046458] kthread+0x84/0x88
[ee071ff0] [c000d434] kernel_thread+0x4c/0x68
Instruction dump:
7c0004ac 7d404c2c 0c0a0000 4c00012c 7c0004ac 7c004c2c 0c000000 4c00012c 
7f8a0000 409effdc 7c0004ac 7c004c2c <0c000000> 4c00012c 7c0004ac 7d604c2c 

I debugged it to the innermost loop of rtc_write_sync_lp(), where
count2 and count3 are always equal.  Should there be a timeout in
that loop?  Do you know if this is because the machine isn't
in trusted mode?  Any other hints appreciated.

This is probably because readl/writel does not work on powerpc.
I experienced the same thing when working on fsl_ssi driver.

My patch only targets imx6q right now.

interesting.  When I add

+#define readl in_be32
+#define writel(val, addr)             out_be32(addr, val)

to the top of the driver, the soft-lockup moves to
rtc_read_lp_counter().  I'm trying to verify if I have the right h/w
revision.

Kim

On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote:

how, without the "simple-bus" addition to the sec_mon compatibles
list in the imx6 device tree (which isn't present in this patch)?

This patch does not present anything about sec_mon node.  It's all
about sec_mon_rtc_lp node.  The driver itself does not know if the
rtc_lp node is a subnode of sec_mon node at all.

Can you please tell me what exact change you expect me to make on
drivers/rtc/rtc-snvs.c?  The patch merely adds a lp-rtc driver and
should not include any platform specific changes (dts, etc.)  The
driver and dts changes are two orthogonal parts, and should go via
RTC tree and ARCH tree separately.

interesting.  When I add

+#define readl in_be32
+#define writel(val, addr)             out_be32(addr, val)

to the top of the driver, the soft-lockup moves to
rtc_read_lp_counter().  I'm trying to verify if I have the right h/w
revision.

You can submit an incremental patch later to make the driver work for
powerpc.

-- 
Regards,
Shawn

On Wed, 11 Jul 2012 10:57:55 +0800
Shawn Guo [off-list ref] wrote:

On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote:

quoted

how, without the "simple-bus" addition to the sec_mon compatibles
list in the imx6 device tree (which isn't present in this patch)?

This patch does not present anything about sec_mon node.

it makes a change to its definition in the device tree documentation.

 It's all about sec_mon_rtc_lp node.

the change is that the sec_mon node now includes the sec_mon_rtc_lp
node, and they are related, just as in h/w.

The driver itself does not know if the
rtc_lp node is a subnode of sec_mon node at all.

I understand that, but as it stands (and AFAICT), this patch by
itself doesn't enable the driver to be probed on _any_ platform,
including ARM.

Can you please tell me what exact change you expect me to make on
drivers/rtc/rtc-snvs.c?  The patch merely adds a lp-rtc driver and
should not include any platform specific changes (dts, etc.)  The
driver and dts changes are two orthogonal parts, and should go via
RTC tree and ARCH tree separately.

That's odd; I would expect all enabling changes to either be
included in the same patch, or at least in the same patchseries, and
one subsystem maintainer ack the other.  If ARM-related patch
processes differ somehow, then so be it.

quoted

interesting.  When I add

+#define readl in_be32
+#define writel(val, addr)             out_be32(addr, val)

to the top of the driver, the soft-lockup moves to
rtc_read_lp_counter().  I'm trying to verify if I have the right h/w
revision.

You can submit an incremental patch later to make the driver work for
powerpc.

Turns out there was a mistake in the QorIQ series documentation
that exposed SRTC registers even though the SNVS LP SRTC isn't
physically present on any of the Power based parts; I've since
notified the documentation team.

Thanks,

Kim

On Wed, Jul 11, 2012 at 08:50:31PM -0500, Kim Phillips wrote:

On Wed, 11 Jul 2012 10:57:55 +0800
Shawn Guo [off-list ref] wrote:

quoted

On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote:

quoted

how, without the "simple-bus" addition to the sec_mon compatibles
list in the imx6 device tree (which isn't present in this patch)?

This patch does not present anything about sec_mon node.

it makes a change to its definition in the device tree documentation.

quoted

 It's all about sec_mon_rtc_lp node.

the change is that the sec_mon node now includes the sec_mon_rtc_lp
node, and they are related, just as in h/w.

I'm primarily talking about drivers/rtc/rtc-snvs.c.  There is no change
required on drivers/rtc/rtc-snvs.c whether the device is created by
a sec_mon driver or "simple-bus" approach.

quoted

The driver itself does not know if the
rtc_lp node is a subnode of sec_mon node at all.

I understand that, but as it stands (and AFAICT), this patch by
itself doesn't enable the driver to be probed on _any_ platform,
including ARM.

It's never a thing that RTC driver should care.  As long as the driver
is verified to be functional, it's platform's call to enable the device
or not on particular board/system.

quoted

Can you please tell me what exact change you expect me to make on
drivers/rtc/rtc-snvs.c?  The patch merely adds a lp-rtc driver and
should not include any platform specific changes (dts, etc.)  The
driver and dts changes are two orthogonal parts, and should go via
RTC tree and ARCH tree separately.

That's odd; I would expect all enabling changes to either be
included in the same patch, or at least in the same patchseries, and
one subsystem maintainer ack the other.  If ARM-related patch
processes differ somehow, then so be it.

I don't think it's an ARM-related process.  The process you mentioned
is only used for particular case where a series touches multiple
subsystems while the whole series has to go via one tree in order to
maintain "git bisect".  Since there is no bisect for a new driver to
maintain at all, it should just follow the general process, having
patches go via respective subsystem tree to minimize the merge conflicts
when different trees get merged together.

-- 
Regards,
Shawn