[PATCH v2 0/5] Add a new RTC driver for recent mvebu SoCs

[PATCH v2 0/5] Add a new RTC driver for recent mvebu SoCs

[Gregory CLEMENT]

Hi,

The Marvell Armada 38x SoCs contains an RTC which differs from the RTC
used in the other mvebu SoCs until now. This second version of the
patch set adds support for this new IP and enable it in the Device
Tree of the Armada 38x SoC.

Thanks,

Gr??gory

Changelog
v1 -> v2:

- Used reg-names to identify the registers in the device tree
- Changed the space into tab in the MAINTAINERS file
- Emphasized that the 5s wait was only needed between two consecutive
  writes
- Reduced the wait in the set_time function, 100ms were enough
- If no interrupt was available, then disable the alarm related
  functions
- Fixed the critical sections
- Updated the mvebu_v7_defconfig by enabling the new RTC support

Gregory CLEMENT (5):
  rtc: armada38x: Add the device tree binding documentation
  drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs
  MAINTAINERS: Add the RTC driver for the Armada38x
  ARM: mvebu: add Device Tree description of RTC on Armada 38x
  ARM: mvebu: enable Armada 38x RTC driver in mvebu_v7_defconfig

 .../devicetree/bindings/rtc/armada-380-rtc.txt     |  22 ++
 MAINTAINERS                                        |   1 +
 arch/arm/boot/dts/armada-38x.dtsi                  |   7 +
 arch/arm/configs/mvebu_v7_defconfig                |   1 +
 drivers/rtc/Kconfig                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-armada38x.c                        | 319 +++++++++++++++++++++
 7 files changed, 361 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
 create mode 100644 drivers/rtc/rtc-armada38x.c

-- 
1.9.1

[PATCH v2 1/5] rtc: armada38x: Add the device tree binding documentation

[Gregory CLEMENT]

The Armada 38x SoCs come with a new RTC which differs from the one
used in the other mvebu SoCs until now. This patch describes the
binding of this RTC.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/rtc/armada-380-rtc.txt     | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/armada-380-rtc.txt b/Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
new file mode 100644
index 000000000000..5a58aa94cfc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
@@ -0,0 +1,22 @@
+* Real Time Clock of te Armada 38x SoCs
+
+RTC controller for the Armada 38x SoCs
+
+Required properties:
+- compatible : Should be "marvell,armada-380-rtc"
+- reg: physical base address of the controller and length of memory
+  mapped region, associated to the reg-name "rtc". The other entry is
+  related to the interrupt control from the SoC, associated to the
+  reg-name "soc-interrupt".
+- reg-names: names of the mapped memory regions listed in regs
+    property in the same order: "rtc" and "soc-int".
+- interrupts: IRQ line for the RTC.
+
+Example:
+
+rtc at a3800 {
+	compatible = "marvell,armada-380-rtc";
+	reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+	reg-names = "rtc", "soc-int";
+	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
1.9.1

[PATCH v2 1/5] rtc: armada38x: Add the device tree binding documentation

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Wed, 14 Jan 2015 18:39:11 +0100, Gregory CLEMENT wrote:

> +Required properties:
> +- compatible : Should be "marvell,armada-380-rtc"
> +- reg: physical base address of the controller and length of memory
> +  mapped region, associated to the reg-name "rtc". The other entry is
> +  related to the interrupt control from the SoC, associated to the
> +  reg-name "soc-interrupt".

soc-interrupt or...

> +- reg-names: names of the mapped memory regions listed in regs
> +    property in the same order: "rtc" and "soc-int".

soc-int ?

> +rtc at a3800 {
> +	compatible = "marvell,armada-380-rtc";
> +	reg = <0xa3800 0x20>, <0x184a0 0x0c>;

Any reason to use <0x184A0 0xC> instead of <0x184A8 0x4> ? According to
the datasheet, there is only this 184A8 register for RTC stuff.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 1/5] rtc: armada38x: Add the device tree binding documentation

[Gregory CLEMENT]

Hi Thomas,

On 14/01/2015 20:22, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Wed, 14 Jan 2015 18:39:11 +0100, Gregory CLEMENT wrote:
> 
>> +Required properties:
>> +- compatible : Should be "marvell,armada-380-rtc"
>> +- reg: physical base address of the controller and length of memory
>> +  mapped region, associated to the reg-name "rtc". The other entry is
>> +  related to the interrupt control from the SoC, associated to the
>> +  reg-name "soc-interrupt".
> 
> soc-interrupt or...

I changed all the name from soc-interrupt to soc-int except here.
I will fix it.

> 
>> +- reg-names: names of the mapped memory regions listed in regs
>> +    property in the same order: "rtc" and "soc-int".
> 
> soc-int ?
> 
>> +rtc at a3800 {
>> +	compatible = "marvell,armada-380-rtc";
>> +	reg = <0xa3800 0x20>, <0x184a0 0x0c>;
> 
> Any reason to use <0x184A0 0xC> instead of <0x184A8 0x4> ? According to
> the datasheet, there is only this 184A8 register for RTC stuff.

Yes but according to the code I saw there were other registers related to the RTC
from 0x184A0. Even if we don't use them now I prefer having an accurate mapping from
the beginning for avoiding using negative offset as we needed to do in the past.

Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 1/5] rtc: armada38x: Add the device tree binding documentation

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 15 Jan 2015 08:50:07 +0100, Gregory CLEMENT wrote:

> > Any reason to use <0x184A0 0xC> instead of <0x184A8 0x4> ? According to
> > the datasheet, there is only this 184A8 register for RTC stuff.
> 
> Yes but according to the code I saw there were other registers related to the RTC
> from 0x184A0. Even if we don't use them now I prefer having an accurate mapping from
> the beginning for avoiding using negative offset as we needed to do in the past.

So I presume you mean that there are undocumented registers, since I
didn't see them in the datasheet. Is this correct?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 1/5] rtc: armada38x: Add the device tree binding documentation

[Gregory CLEMENT]

Hi Thomas,

On 15/01/2015 09:24, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 15 Jan 2015 08:50:07 +0100, Gregory CLEMENT wrote:
> 
>>> Any reason to use <0x184A0 0xC> instead of <0x184A8 0x4> ? According to
>>> the datasheet, there is only this 184A8 register for RTC stuff.
>>
>> Yes but according to the code I saw there were other registers related to the RTC
>> from 0x184A0. Even if we don't use them now I prefer having an accurate mapping from
>> the beginning for avoiding using negative offset as we needed to do in the past.
> 
> So I presume you mean that there are undocumented registers, since I
> didn't see them in the datasheet. Is this correct?

Yes indeed there are not documented in the current version of our datasheet.

Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 2/5] drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

[Gregory CLEMENT]

The new mvebu SoCs come with a new RTC driver. This patch adds the
support for this new IP which is currently found in the Armada 38x
SoCs.

This RTC provides two alarms, but only the first one is used in the
driver. The RTC also allows using periodic interrupts.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/rtc/Kconfig         |  10 ++
 drivers/rtc/Makefile        |   1 +
 drivers/rtc/rtc-armada38x.c | 319 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/rtc/rtc-armada38x.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index f15cddfeb897..de42ebd4b626 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1269,6 +1269,16 @@ config RTC_DRV_MV
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-mv.
 
+config RTC_DRV_ARMADA38X
+	tristate "Armada 38x Marvell SoC RTC"
+	depends on ARCH_MVEBU
+	help
+	  If you say yes here you will get support for the in-chip RTC
+	  that can be found in the Armada 38x Marvell's SoC device
+
+	  This driver can also be built as a module. If so, the module
+	  will be called armada38x-rtc.
+
 config RTC_DRV_PS3
 	tristate "PS3 RTC"
 	depends on PPC_PS3
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index c8ef3e1e6ccd..03fe5629c464 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_RTC_DRV_88PM860X)  += rtc-88pm860x.o
 obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
 obj-$(CONFIG_RTC_DRV_AB3100)	+= rtc-ab3100.o
 obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
+obj-$(CONFIG_RTC_DRV_ARMADA38X)	+= rtc-armada38x.o
 obj-$(CONFIG_RTC_DRV_AS3722)	+= rtc-as3722.o
 obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
 obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
new file mode 100644
index 000000000000..30bae27e828c
--- /dev/null
+++ b/drivers/rtc/rtc-armada38x.c
@@ -0,0 +1,319 @@
+/*
+ * RTC driver for the Armada 38x Marvell SoCs
+ *
+ * Copyright (C) 2015 Marvell
+ *
+ * Gregory Clement <gregory.clement@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#define RTC_STATUS	    0x0
+#define RTC_STATUS_ALARM1	    BIT(0)
+#define RTC_STATUS_ALARM2	    BIT(1)
+#define RTC_IRQ1_CONF	    0x4
+#define RTC_IRQ1_AL_EN		    BIT(0)
+#define RTC_IRQ1_FREQ_EN	    BIT(1)
+#define RTC_IRQ1_FREQ_1HZ	    BIT(2)
+#define RTC_TIME	    0xC
+#define RTC_ALARM1	    0x10
+
+#define SOC_RTC_INTERRUPT   0x8
+#define SOC_RTC_ALARM1		BIT(0)
+#define SOC_RTC_ALARM2		BIT(1)
+#define SOC_RTC_ALARM1_MASK	BIT(2)
+#define SOC_RTC_ALARM2_MASK	BIT(3)
+
+struct armada38x_rtc {
+	struct rtc_device   *rtc_dev;
+	void __iomem	    *regs;
+	void __iomem	    *regs_soc;
+	spinlock_t	    lock;
+	int		    irq;
+};
+
+/*
+ * According to the datasheet, we need to wait for 5us only between
+ * two consecutive writes
+ */
+static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
+{
+	writel(val, rtc->regs + offset);
+	udelay(5);
+}
+
+static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long time, time_check, flags;
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	time = readl(rtc->regs + RTC_TIME);
+	/*
+	 * WA for failing time set attempts. As stated in HW ERRATA if
+	 * more than one second between two time reads is detected
+	 * then read once again.
+	 */
+	time_check = readl(rtc->regs + RTC_TIME);
+	if ((time_check - time) > 1)
+		time_check = readl(rtc->regs + RTC_TIME);
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	rtc_time_to_tm(time_check, tm);
+
+	return 0;
+}
+
+static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	int ret = 0;
+	unsigned long time, flags;
+
+	ret = rtc_tm_to_time(tm, &time);
+
+	if (ret)
+		goto out;
+	/*
+	 * Setting the RTC time not always succeeds. According to the
+	 * errata we need to first write on the status register and
+	 * then wait for 100ms before writing to the time register to be
+	 * sure that the data will be taken into account.
+	 */
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	writel(0, rtc->regs + RTC_STATUS);
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	msleep(100);
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	writel(time, rtc->regs + RTC_TIME);
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+out:
+	return ret;
+}
+
+static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long time, flags;
+	u32 val;
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	time = readl(rtc->regs + RTC_ALARM1);
+	val = readl(rtc->regs + RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	alrm->enabled = val ? 1 : 0;
+	rtc_time_to_tm(time,  &alrm->time);
+
+	return 0;
+}
+
+static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long time, flags;
+	int ret = 0;
+	u32 val;
+
+	ret = rtc_tm_to_time(&alrm->time, &time);
+
+	if (ret)
+		goto out;
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	rtc_delayed_write(time, rtc, RTC_ALARM1);
+
+	if (alrm->enabled) {
+			rtc_delayed_write(RTC_IRQ1_AL_EN , rtc, RTC_IRQ1_CONF);
+			val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
+		writel(val | SOC_RTC_ALARM1_MASK,
+			rtc->regs_soc + SOC_RTC_INTERRUPT);
+	}
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+out:
+	return ret;
+}
+
+static int armada38x_rtc_alarm_irq_enable(struct device *dev,
+					 unsigned int enabled)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	if (enabled)
+		writel(RTC_IRQ1_AL_EN, rtc->regs + RTC_IRQ1_CONF);
+	else
+		writel(0, rtc->regs + RTC_IRQ1_CONF);
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	return 0;
+}
+
+static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
+{
+	struct armada38x_rtc *rtc = data;
+	u32 val;
+	int event = RTC_IRQF | RTC_AF;
+
+	dev_dbg(&rtc->rtc_dev->dev, "%s:irq(%d)\n", __func__, irq);
+
+	spin_lock(&rtc->lock);
+
+	val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
+
+	writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);
+	val = readl(rtc->regs + RTC_IRQ1_CONF);
+	/* disable all the interrupts for alarm 1 */
+	rtc_delayed_write(0, rtc, RTC_IRQ1_CONF);
+	/* Ack the event */
+	writel(RTC_STATUS_ALARM1, rtc->regs + RTC_STATUS);
+
+	spin_unlock(&rtc->lock);
+
+	if (val & RTC_IRQ1_FREQ_EN) {
+		if (val & RTC_IRQ1_FREQ_1HZ)
+			event |= RTC_UF;
+		else
+			event |= RTC_PF;
+	}
+
+	rtc_update_irq(rtc->rtc_dev, 1, event);
+
+	return IRQ_HANDLED;
+}
+
+static struct rtc_class_ops armada38x_rtc_ops = {
+	.read_time = armada38x_rtc_read_time,
+	.set_time = armada38x_rtc_set_time,
+	.read_alarm = armada38x_rtc_read_alarm,
+	.set_alarm = armada38x_rtc_set_alarm,
+	.alarm_irq_enable = armada38x_rtc_alarm_irq_enable,
+};
+
+static __init int armada38x_rtc_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct armada38x_rtc *rtc;
+	int ret;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(struct armada38x_rtc),
+			    GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	spin_lock_init(&rtc->lock);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rtc");
+	rtc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rtc->regs))
+		return PTR_ERR(rtc->regs);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "soc-int");
+	rtc->regs_soc = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rtc->regs_soc))
+		return PTR_ERR(rtc->regs_soc);
+
+	rtc->irq = platform_get_irq(pdev, 0);
+
+	if (rtc->irq < 0) {
+		dev_err(&pdev->dev, "no irq\n");
+		return rtc->irq;
+	}
+	if (devm_request_irq(&pdev->dev, rtc->irq, armada38x_rtc_alarm_irq,
+				0, pdev->name, rtc) < 0) {
+		dev_warn(&pdev->dev, "Interrupt not available.\n");
+		rtc->irq = -1;
+		/*
+		 * If there is no interrupt available then we can't
+		 * use the alarm
+		 */
+		armada38x_rtc_ops.set_alarm = NULL;
+		armada38x_rtc_ops.alarm_irq_enable = NULL;
+	}
+	platform_set_drvdata(pdev, rtc);
+	if (rtc->irq != -1)
+		device_init_wakeup(&pdev->dev, 1);
+
+	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
+					&armada38x_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc->rtc_dev)) {
+		ret = PTR_ERR(rtc->rtc_dev);
+		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
+		return ret;
+	}
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int armada38x_rtc_suspend(struct device *dev)
+{
+	if (device_may_wakeup(dev)) {
+		struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+
+		return enable_irq_wake(rtc->irq);
+	}
+
+	return 0;
+}
+
+static int armada38x_rtc_resume(struct device *dev)
+{
+	if (device_may_wakeup(dev)) {
+		struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+
+		return disable_irq_wake(rtc->irq);
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(armada38x_rtc_pm_ops,
+			 armada38x_rtc_suspend, armada38x_rtc_resume);
+
+#ifdef CONFIG_OF
+static const struct of_device_id armada38x_rtc_of_match_table[] = {
+	{ .compatible = "marvell,armada-380-rtc", },
+	{}
+};
+#endif
+
+static struct platform_driver armada38x_rtc_driver = {
+	.driver		= {
+		.name	= "armada38x-rtc",
+		.pm	= &armada38x_rtc_pm_ops,
+		.of_match_table = of_match_ptr(armada38x_rtc_of_match_table),
+	},
+};
+
+module_platform_driver_probe(armada38x_rtc_driver, armada38x_rtc_probe);
+
+MODULE_DESCRIPTION("Marvell Armada 38x RTC driver");
+MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v2 2/5] drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

[Arnaud Ebalard]

Hi Gregory,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> The new mvebu SoCs come with a new RTC driver. This patch adds the
> support for this new IP which is currently found in the Armada 38x
> SoCs.
>
> This RTC provides two alarms, but only the first one is used in the
> driver. The RTC also allows using periodic interrupts.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/rtc/Kconfig         |  10 ++
>  drivers/rtc/Makefile        |   1 +
>  drivers/rtc/rtc-armada38x.c | 319 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 330 insertions(+)
>  create mode 100644 drivers/rtc/rtc-armada38x.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index f15cddfeb897..de42ebd4b626 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1269,6 +1269,16 @@ config RTC_DRV_MV
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-mv.
>  
> +config RTC_DRV_ARMADA38X
> +	tristate "Armada 38x Marvell SoC RTC"
> +	depends on ARCH_MVEBU
> +	help
> +	  If you say yes here you will get support for the in-chip RTC
> +	  that can be found in the Armada 38x Marvell's SoC device
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called armada38x-rtc.
> +
>  config RTC_DRV_PS3
>  	tristate "PS3 RTC"
>  	depends on PPC_PS3
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index c8ef3e1e6ccd..03fe5629c464 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_RTC_DRV_88PM860X)  += rtc-88pm860x.o
>  obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
>  obj-$(CONFIG_RTC_DRV_AB3100)	+= rtc-ab3100.o
>  obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
> +obj-$(CONFIG_RTC_DRV_ARMADA38X)	+= rtc-armada38x.o
>  obj-$(CONFIG_RTC_DRV_AS3722)	+= rtc-as3722.o
>  obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
>  obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> new file mode 100644
> index 000000000000..30bae27e828c
> --- /dev/null
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -0,0 +1,319 @@
> +/*
> + * RTC driver for the Armada 38x Marvell SoCs
> + *
> + * Copyright (C) 2015 Marvell
> + *
> + * Gregory Clement <gregory.clement@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +
> +#define RTC_STATUS	    0x0
> +#define RTC_STATUS_ALARM1	    BIT(0)
> +#define RTC_STATUS_ALARM2	    BIT(1)
> +#define RTC_IRQ1_CONF	    0x4
> +#define RTC_IRQ1_AL_EN		    BIT(0)
> +#define RTC_IRQ1_FREQ_EN	    BIT(1)
> +#define RTC_IRQ1_FREQ_1HZ	    BIT(2)
> +#define RTC_TIME	    0xC
> +#define RTC_ALARM1	    0x10
> +
> +#define SOC_RTC_INTERRUPT   0x8
> +#define SOC_RTC_ALARM1		BIT(0)
> +#define SOC_RTC_ALARM2		BIT(1)
> +#define SOC_RTC_ALARM1_MASK	BIT(2)
> +#define SOC_RTC_ALARM2_MASK	BIT(3)
> +
> +struct armada38x_rtc {
> +	struct rtc_device   *rtc_dev;
> +	void __iomem	    *regs;
> +	void __iomem	    *regs_soc;
> +	spinlock_t	    lock;
> +	int		    irq;
> +};
> +
> +/*
> + * According to the datasheet, we need to wait for 5us only between
> + * two consecutive writes
> + */
> +static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
> +{
> +	writel(val, rtc->regs + offset);
> +	udelay(5);
> +}

The thing I do not get is how you can guarantee that an undelayed
writel() in a function will not be followed less than 5?s later by
another writel() in another function. For instance:

  1) a call to set_alarm() followed by a call to alarm_irq_enable().
  2) some writel() or even rtc_udelayed_write() w/ sth asynchronous
    occuring like your interrupt handler just after that.

I guess it may be possible to make assumptions on the fact that the
amount of code before a writel() or rtc_udelayed_write() may prevent
such scenario but it's difficult to tell, all the more w/ a spinlock
before the writel() in irq_enable().

Considering the description of the bug, why not doing the following:

 1) implement rtc_delayed_write() a bit differently:

    static inline void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
    {
	udelay(5);
	writel(val, rtc->regs + offset);
    }

 2) remove all writel() in the driver and use rtc_delayed_write()
    everywhere.

All writes being under the protection of your spinlock, this will
guarantee the 5?s delay in all cases.


> +static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
> +	unsigned long time, time_check, flags;
> +
> +	spin_lock_irqsave(&rtc->lock, flags);
> +
> +	time = readl(rtc->regs + RTC_TIME);
> +	/*
> +	 * WA for failing time set attempts. As stated in HW ERRATA if
> +	 * more than one second between two time reads is detected
> +	 * then read once again.
> +	 */
> +	time_check = readl(rtc->regs + RTC_TIME);
> +	if ((time_check - time) > 1)
> +		time_check = readl(rtc->regs + RTC_TIME);
> +
> +	spin_unlock_irqrestore(&rtc->lock, flags);
> +
> +	rtc_time_to_tm(time_check, tm);
> +
> +	return 0;
> +}
> +
> +static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
> +	int ret = 0;
> +	unsigned long time, flags;
> +
> +	ret = rtc_tm_to_time(tm, &time);
> +
> +	if (ret)
> +		goto out;
> +	/*
> +	 * Setting the RTC time not always succeeds. According to the
> +	 * errata we need to first write on the status register and
> +	 * then wait for 100ms before writing to the time register to be
> +	 * sure that the data will be taken into account.
> +	 */
> +	spin_lock_irqsave(&rtc->lock, flags);
> +
> +	writel(0, rtc->regs + RTC_STATUS);
> +
> +	spin_unlock_irqrestore(&rtc->lock, flags);
> +
> +	msleep(100);
> +
> +	spin_lock_irqsave(&rtc->lock, flags);
> +
> +	writel(time, rtc->regs + RTC_TIME);

probably not critical (it's rtc clock and not system clock) but you still
introduce a 100ms shift when setting time here.

As for the two writel() not being done w/o releasing the spinlock, it
looks a bit weird but it seems it's ok for other functions manipulating
RTC_STATUS reg. 

Nonetheless, in the reverse direction, if a writel() occurs somewhere
(e.g. in the alarm irq handler) during your msleep(), you may do your
final writel() w/o having enforced the expected 100ms delay.


> [SNIP]
>

[PATCH v2 2/5] drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

[Gregory CLEMENT]

Hi Arnaud,
[...]

>> +/*
>> + * According to the datasheet, we need to wait for 5us only between
>> + * two consecutive writes
>> + */
>> +static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
>> +{
>> +	writel(val, rtc->regs + offset);
>> +	udelay(5);
>> +}
> 
> The thing I do not get is how you can guarantee that an undelayed
> writel() in a function will not be followed less than 5?s later by
> another writel() in another function. For instance:
> 
>   1) a call to set_alarm() followed by a call to alarm_irq_enable().
>   2) some writel() or even rtc_udelayed_write() w/ sth asynchronous
>     occuring like your interrupt handler just after that.
> 
> I guess it may be possible to make assumptions on the fact that the
> amount of code before a writel() or rtc_udelayed_write() may prevent
> such scenario but it's difficult to tell, all the more w/ a spinlock
> before the writel() in irq_enable().
> 
> Considering the description of the bug, why not doing the following:
> 
>  1) implement rtc_delayed_write() a bit differently:
> 
>     static inline void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
>     {
> 	udelay(5);
> 	writel(val, rtc->regs + offset);
>     }
> 
>  2) remove all writel() in the driver and use rtc_delayed_write()
>     everywhere.
> 
> All writes being under the protection of your spinlock, this will
> guarantee the 5?s delay in all cases.

You're right. I tried avoiding loosing microseconds here and there, but there
is too many case where it can fail. So let's us it everywhere.

[...]

>> +static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> +	int ret = 0;
>> +	unsigned long time, flags;
>> +
>> +	ret = rtc_tm_to_time(tm, &time);
>> +
>> +	if (ret)
>> +		goto out;
>> +	/*
>> +	 * Setting the RTC time not always succeeds. According to the
>> +	 * errata we need to first write on the status register and
>> +	 * then wait for 100ms before writing to the time register to be
>> +	 * sure that the data will be taken into account.
>> +	 */
>> +	spin_lock_irqsave(&rtc->lock, flags);
>> +
>> +	writel(0, rtc->regs + RTC_STATUS);
>> +
>> +	spin_unlock_irqrestore(&rtc->lock, flags);
>> +
>> +	msleep(100);
>> +
>> +	spin_lock_irqsave(&rtc->lock, flags);
>> +
>> +	writel(time, rtc->regs + RTC_TIME);
> 
> probably not critical (it's rtc clock and not system clock) but you still
> introduce a 100ms shift when setting time here.

I am aware of this shift but the granularity is the second, so we can't do
better, we have to deal with the limitation of the hardware. :(

> 
> As for the two writel() not being done w/o releasing the spinlock, it
> looks a bit weird but it seems it's ok for other functions manipulating
> RTC_STATUS reg. 
> 
> Nonetheless, in the reverse direction, if a writel() occurs somewhere
> (e.g. in the alarm irq handler) during your msleep(), you may do your
> final writel() w/o having enforced the expected 100ms delay.

Actually I don't know if it is problematic if a writel occur in an other register
during the 100ms. I still wait for an answer for it.


Thanks,

Gregory




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 3/5] MAINTAINERS: Add the RTC driver for the Armada38x

[Gregory CLEMENT]

Put it in the mvebu entry.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ddb9ac8d32b3..393f158a64e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1157,6 +1157,7 @@ M:	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
 L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	arch/arm/mach-mvebu/
+F:	drivers/rtc/armada38x-rtc
 
 ARM/Marvell Berlin SoC support
 M:	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
-- 
1.9.1

[PATCH v2 4/5] ARM: mvebu: add Device Tree description of RTC on Armada 38x

[Gregory CLEMENT]

The Marvell Armada 38x SoCs contains an RTC which differs from the RTC
used in the other mvebu SoCs until now. This commit adds the Device
Tree description of this interface at the SoC level.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-38x.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index 04fe80d101f8..36c1d35873dd 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -466,6 +466,13 @@
 				clocks = <&gateclk 4>;
 			};
 
+			rtc at a3800 {
+				compatible = "marvell,armada-380-rtc";
+				reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+				reg-names = "rtc", "soc-int";
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
 			sata at a8000 {
 				compatible = "marvell,armada-380-ahci";
 				reg = <0xa8000 0x2000>;
-- 
1.9.1

[PATCH v2 5/5] ARM: mvebu: enable Armada 38x RTC driver in mvebu_v7_defconfig

[Gregory CLEMENT]

Now that the Armada 38x RTC driver has been pushed, let's enable it in
mvebu_v7_defconfig.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/configs/mvebu_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/mvebu_v7_defconfig b/arch/arm/configs/mvebu_v7_defconfig
index 627accea72fb..2400b9f52403 100644
--- a/arch/arm/configs/mvebu_v7_defconfig
+++ b/arch/arm/configs/mvebu_v7_defconfig
@@ -112,6 +112,7 @@ CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_S35390A=y
 CONFIG_RTC_DRV_MV=y
+CONFIG_RTC_DRV_ARMADA38X=y
 CONFIG_DMADEVICES=y
 CONFIG_MV_XOR=y
 # CONFIG_IOMMU_SUPPORT is not set
-- 
1.9.1