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

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

[Shawn Guo]

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

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
---
 Documentation/devicetree/bindings/rtc/snvs-rtc.txt |   19 +
 drivers/rtc/Kconfig                                |   10 +
 drivers/rtc/Makefile                               |    1 +
 drivers/rtc/rtc-snvs.c                             |  350 ++++++++++++++++++++
 4 files changed, 380 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/rtc/snvs-rtc.txt b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt
new file mode 100644
index 0000000..18e7c5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt
@@ -0,0 +1,19 @@
+* Freescale Secure Non-Volatile Storage (SNVS) RTC
+
+Required properties:
+- compatible: Should be "fsl,sec-v4.0-mon"
+  See Documentation/devicetree/bindings/crypto/fsl-sec4.txt for details.
+- reg: Physical base address of the controller and length of memory mapped
+  region.
+- interrupts: Two interrupt numbers to the cpu should be specified. First
+  interrupt number is the consolidated interrupt in non TrustZone. The second
+  interrupt number is the security interrupt in TrustZone. The number of cells
+  representing a interrupt depends on the parent interrupt controller.
+
+Example:
+
+snvs at 020cc000 {
+	compatible = "fsl,sec-v4.0-mon";
+	reg = <0x020cc000 0x4000>;
+	interrupts = <0 19 0x04 0 20 0x04>;
+};
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 08cbdb9..511ddcb 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1087,4 +1087,14 @@ 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 ARCH_MXC
+	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..761c91f
--- /dev/null
+++ b/drivers/rtc/rtc-snvs.c
@@ -0,0 +1,350 @@
+/*
+ * 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/delay.h>
+#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>
+
+#define SNVS_LPCR		0x38
+#define SNVS_LPSR		0x4c
+#define SNVS_LPSRTCMR		0x50
+#define SNVS_LPSRTCLR		0x54
+#define SNVS_LPTAR		0x58
+#define SNVS_LPPGDR		0x64
+
+#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 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);
+
+	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,
+			       0, "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 platform_device *pdev, pm_message_t state)
+{
+	struct snvs_rtc_data *data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev))
+		enable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static int snvs_rtc_resume(struct platform_device *pdev)
+{
+	struct snvs_rtc_data *data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev))
+		disable_irq_wake(data->irq);
+
+	return 0;
+}
+#else
+#define snvs_rtc_suspend NULL
+#define snvs_rtc_resume NULL
+#endif
+
+static const struct of_device_id __devinitdata snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, snvs_dt_ids);
+
+static struct platform_driver snvs_rtc_driver = {
+	.driver = {
+		.name	= "snvs_rtc",
+		.owner	= THIS_MODULE,
+		.of_match_table = snvs_dt_ids,
+	},
+	.probe		= snvs_rtc_probe,
+	.remove		= __devexit_p(snvs_rtc_remove),
+	.suspend	= snvs_rtc_suspend,
+	.resume		= snvs_rtc_resume,
+};
+module_platform_driver(snvs_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Freescale SNVS RTC Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

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

[Stephen Boyd]

On 07/02/12 09:09, Shawn Guo wrote:
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 08cbdb9..511ddcb 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1087,4 +1087,14 @@ 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 ARCH_MXC

If you want more build coverage you can depend on HAS_IOMEM and drop the
ARCH_MXC part.

> +	help
> +	   If you say yes here you get support for the Freescale SNVS
> +	   Low Power (LP) RTC module.
> +
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> new file mode 100644
> index 0000000..761c91f
> --- /dev/null
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -0,0 +1,350 @@
> +/*
> + * 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/delay.h>

Is this include used?

> +
> +static struct rtc_class_ops snvs_rtc_ops = {

const?

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

Why not just pass the svsns_rtc_data to request_irq() so you can cast
dev_id directly?

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

What if this irq fails to exist?


> +static const struct of_device_id __devinitdata snvs_dt_ids[] = {

Is this supposed to be __devinitconst?

> +	{ .compatible = "fsl,sec-v4.0-mon" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, snvs_dt_ids);

Surround this of_device table in #ifdef CONFIG_OF?

> +
> +static struct platform_driver snvs_rtc_driver = {
> +	.driver = {
> +		.name	= "snvs_rtc",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = snvs_dt_ids,

.of_match_table = of_match_ptr(svns_dt_ids)

> +	},
> +	.probe		= snvs_rtc_probe,
> +	.remove		= __devexit_p(snvs_rtc_remove),
> +	.suspend	= snvs_rtc_suspend,
> +	.resume		= snvs_rtc_resume,

Can you use dev_pm_ops?

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

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

[Shawn Guo]

Thanks for looking at it, Stephen.

On Mon, Jul 02, 2012 at 12:07:00PM -0700, Stephen Boyd wrote:
> On 07/02/12 09:09, Shawn Guo wrote:
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index 08cbdb9..511ddcb 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -1087,4 +1087,14 @@ 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 ARCH_MXC
> 
> If you want more build coverage you can depend on HAS_IOMEM and drop the
> ARCH_MXC part.
> 
Dropping ARCH_MXC sounds like a good idea.  But why do we need to
depend on HAS_IOMEM?  readl/writel are not always available?

I would add "depends on OF" though.

> > +	help
> > +	   If you say yes here you get support for the Freescale SNVS
> > +	   Low Power (LP) RTC module.
> > +
> > diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> > new file mode 100644
> > index 0000000..761c91f
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-snvs.c
> > @@ -0,0 +1,350 @@
> > +/*
> > + * 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/delay.h>
> 
> Is this include used?
> 
No.  It should be removed.

> > +
> > +static struct rtc_class_ops snvs_rtc_ops = {
> 
> const?
> 
Indeed.

> > +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);
> 
> Why not just pass the svsns_rtc_data to request_irq() so you can cast
> dev_id directly?
> 
Because snvs_rtc_alarm_irq_enable(dev, 0) is being called in there
with dev as the first parameter.

> > +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);
> 
> What if this irq fails to exist?
> 
The later devm_request_irq() call will report error then.

> 
> > +static const struct of_device_id __devinitdata snvs_dt_ids[] = {
> 
> Is this supposed to be __devinitconst?
> 
Right.

> > +	{ .compatible = "fsl,sec-v4.0-mon" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, snvs_dt_ids);
> 
> Surround this of_device table in #ifdef CONFIG_OF?
> 
Maybe not.  I intend to have the driver OF only.

> > +
> > +static struct platform_driver snvs_rtc_driver = {
> > +	.driver = {
> > +		.name	= "snvs_rtc",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = snvs_dt_ids,
> 
> .of_match_table = of_match_ptr(svns_dt_ids)
> 
Ditto

> > +	},
> > +	.probe		= snvs_rtc_probe,
> > +	.remove		= __devexit_p(snvs_rtc_remove),
> > +	.suspend	= snvs_rtc_suspend,
> > +	.resume		= snvs_rtc_resume,
> 
> Can you use dev_pm_ops?
> 
Ok.

-- 
Regards,
Shawn

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

[Stephen Boyd]

On 07/03/12 07:02, Shawn Guo wrote:
> On Mon, Jul 02, 2012 at 12:07:00PM -0700, Stephen Boyd wrote:
>> On 07/02/12 09:09, Shawn Guo wrote:
>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>> index 08cbdb9..511ddcb 100644
>>> --- a/drivers/rtc/Kconfig
>>> +++ b/drivers/rtc/Kconfig
>>> @@ -1087,4 +1087,14 @@ 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 ARCH_MXC
>> If you want more build coverage you can depend on HAS_IOMEM and drop the
>> ARCH_MXC part.
>>
> Dropping ARCH_MXC sounds like a good idea.  But why do we need to
> depend on HAS_IOMEM?  readl/writel are not always available?

Yes, that is my understanding.

>
> I would add "depends on OF" though.
>
>
>>> +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);
>> Why not just pass the svsns_rtc_data to request_irq() so you can cast
>> dev_id directly?
>>
> Because snvs_rtc_alarm_irq_enable(dev, 0) is being called in there
> with dev as the first parameter.

Ah, sorry for the noise.

>
>>> +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);
>> What if this irq fails to exist?
>>
> The later devm_request_irq() call will report error then.

request_irq() takes an unsigned int. Are you relying on there being no
descriptor for that extremely high irq number? Or maybe I'm missing
something in the code.

>
>>> +	{ .compatible = "fsl,sec-v4.0-mon" },
>>> +	{ /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, snvs_dt_ids);
>> Surround this of_device table in #ifdef CONFIG_OF?
>>
> Maybe not.  I intend to have the driver OF only.
>

Ok. I guess if someone wants to use it non-OF they can always send a patch.

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

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

[Kim Phillips]

On Tue, 3 Jul 2012 00:09:15 +0800
Shawn Guo <shawn.guo@linaro.org> wrote:

>  create mode 100644 Documentation/devicetree/bindings/rtc/snvs-rtc.txt
>  create mode 100644 drivers/rtc/rtc-snvs.c
> 
> diff --git a/Documentation/devicetree/bindings/rtc/snvs-rtc.txt b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt
> new file mode 100644
> index 0000000..18e7c5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/snvs-rtc.txt
> @@ -0,0 +1,19 @@
> +* Freescale Secure Non-Volatile Storage (SNVS) RTC
> +
> +Required properties:
> +- compatible: Should be "fsl,sec-v4.0-mon"
> +  See Documentation/devicetree/bindings/crypto/fsl-sec4.txt for details.
> +- reg: Physical base address of the controller and length of memory mapped
> +  region.
> +- interrupts: Two interrupt numbers to the cpu should be specified. First
> +  interrupt number is the consolidated interrupt in non TrustZone. The second
> +  interrupt number is the security interrupt in TrustZone. The number of cells
> +  representing a interrupt depends on the parent interrupt controller.
> +
> +Example:
> +
> +snvs at 020cc000 {
> +	compatible = "fsl,sec-v4.0-mon";
> +	reg = <0x020cc000 0x4000>;
> +	interrupts = <0 19 0x04 0 20 0x04>;
> +};

Instead of duplicating the binding documentation entry, can this be
merged in with crypto/fsl-sec4.txt?  Also, don't we need the rtc to
be a 'sub-device' of "fsl,sec-vX.Y-mon", and to have its own
compatible, e.g., "fsl,sec-v4.0-mon-rtc-lp"?

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 08cbdb9..511ddcb 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1087,4 +1087,14 @@ 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 ARCH_MXC

should also depend on || FSL_SOC - it was mentioned in the
review of the last version of this patch that this h/w exists on
other, non-ARM based, SoCs.

Kim

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

[Shawn Guo]

On Mon, Jul 02, 2012 at 06:37:36PM -0500, Kim Phillips wrote:
> Instead of duplicating the binding documentation entry, can this be
> merged in with crypto/fsl-sec4.txt?

Yes. We can do that.  But I would still keep a plain
Documentation/devicetree/bindings/rtc/snvs-rtc.txt there with only
a pointer to Documentation/devicetree/bindings/crypto/fsl-sec4.txt.
Otherwise, I'm afraid that people will hardly know the bindings of
snvs-rtc is documented in fsl-sec4.txt.

> Also, don't we need the rtc to
> be a 'sub-device' of "fsl,sec-vX.Y-mon", and to have its own
> compatible, e.g., "fsl,sec-v4.0-mon-rtc-lp"?
> 
Sounds sensible.  But how will reg and interrupts properties be
documented?  The registers for lp-rtc is not so consecutive and
interrupts are shared by hp and lp.

> > +config RTC_DRV_SNVS
> > +	tristate "Freescale SNVS RTC support"
> > +	depends on ARCH_MXC
> 
> should also depend on || FSL_SOC - it was mentioned in the
> review of the last version of this patch that this h/w exists on
> other, non-ARM based, SoCs.
> 
I would not do that until the driver is tested on FSL_SOC.  Also I'm
considering to take Stephen's suggestion to drop even ARCH_MXC.

-- 
Regards,
Shawn

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

[Kim Phillips]

On Tue, 3 Jul 2012 23:04:08 +0800
Shawn Guo <shawn.guo@linaro.org> wrote:

> On Mon, Jul 02, 2012 at 06:37:36PM -0500, Kim Phillips wrote:
> > Instead of duplicating the binding documentation entry, can this be
> > merged in with crypto/fsl-sec4.txt?
> 
> Yes. We can do that.  But I would still keep a plain
> Documentation/devicetree/bindings/rtc/snvs-rtc.txt there with only
> a pointer to Documentation/devicetree/bindings/crypto/fsl-sec4.txt.
> Otherwise, I'm afraid that people will hardly know the bindings of
> snvs-rtc is documented in fsl-sec4.txt.

that's understandable.

> > Also, don't we need the rtc to
> > be a 'sub-device' of "fsl,sec-vX.Y-mon", and to have its own
> > compatible, e.g., "fsl,sec-v4.0-mon-rtc-lp"?
> > 
> Sounds sensible.  But how will reg and interrupts properties be
> documented?  The registers for lp-rtc is not so consecutive and

LP registers go from 0x34 (LPLR) to 0x8b (LPZMKR7), consecutively.
HP registers technically would have a split range, but this patch
doesn't address the HP part.

> interrupts are shared by hp and lp.

that's why we return IRQ_NONE, so we can request_irq IRQF_SHARED.

> > > +config RTC_DRV_SNVS
> > > +	tristate "Freescale SNVS RTC support"
> > > +	depends on ARCH_MXC
> > 
> > should also depend on || FSL_SOC - it was mentioned in the
> > review of the last version of this patch that this h/w exists on
> > other, non-ARM based, SoCs.
> > 
> I would not do that until the driver is tested on FSL_SOC.

I can test it.

> Also I'm
> considering to take Stephen's suggestion to drop even ARCH_MXC.

fine with me.

Kim