Skip to contentSkip to conversation

Add a new RTC driver for recent mvebu SoCs

PATCH Superseded Gregory CLEMENT 4 participants 14 messages Jan 9, 2015 – Jan 14, 2015

Subsystems (3)
Device tree bindings Real time clock (RTC) Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Gregory CLEMENT

author
#0 lore raw

cover Add a new RTC driver for recent mvebu SoCs

Hi,

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

Thanks,

Gr??gory

Gregory CLEMENT (4):
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

 .../devicetree/bindings/rtc/armada-380-rtc.txt     |  17 ++
 MAINTAINERS                                        |   1 +
 arch/arm/boot/dts/armada-38x.dtsi                  |   6 +
 drivers/rtc/Kconfig                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-armada38x.c                        | 305 +++++++++++++++++++++
 6 files changed, 340 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
 create mode 100644 drivers/rtc/rtc-armada38x.c
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/4 rtc: armada38x: Add the device tree binding documentation

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 <[email protected]>
---
 .../devicetree/bindings/rtc/armada-380-rtc.txt          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +17 -0
new file mode 100644
index 000000000000..2c56bef9dd7e
@@ -0,0 +1,17 @@
+* 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. The second entires is for the IP configuration part.
+- interrupts: IRQ line for the RTC.
+
+Example:
+
+rtc at 184a8 {
+	compatible = "marvell,armada-380-rtc";
+	reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+};
Signature · 1 line
1.9.1
#2 lore raw

PATCH 1/4 rtc: armada38x: Add the device tree binding documentation

Hi Gregory,

Quoted 16 lines · 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 <[email protected]>
---
 .../devicetree/bindings/rtc/armada-380-rtc.txt          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +0 -0
new file mode 100644
index 000000000000..2c56bef9dd7e

Documentation/devicetree/bindings/rtc/armada-380-rtc.txt line 8

Quoted 8 lines
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +8 -0
@@ -0,0 +1,17 @@
+* 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. The second entires is for the IP configuration part.

                        ^^^^^^^
                        entry

I had to read probe function to understand precisely the purpose of each region, i.e. IMHO the description could be made a bit clearer.

Quoted 10 lines
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +9 -0
+- interrupts: IRQ line for the RTC.
 
+
+Example:
+
+rtc at 184a8 {
+	compatible = "marvell,armada-380-rtc";
+	reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+};
#3 lore raw

PATCH 1/4 rtc: armada38x: Add the device tree binding documentation

Quoted 31 lines · Arnaud Ebalard

Hi Gregory,

Quoted 16 lines · 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 <[email protected]>
---
 .../devicetree/bindings/rtc/armada-380-rtc.txt          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +0 -0
new file mode 100644
index 000000000000..2c56bef9dd7e

Documentation/devicetree/bindings/rtc/armada-380-rtc.txt line 8

Quoted 8 lines
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +8 -0
@@ -0,0 +1,17 @@
+* 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. The second entires is for the IP configuration part.

                        ^^^^^^^
                        entry
Quoted 2 lines

I had to read probe function to understand precisely the purpose of each region, i.e. IMHO the description could be made a bit clearer.

Maybe using reg-names would make it easier to understand, both in the documentation and the DT itself (as well as not needing to enforce a particular ordering of the two areas in the documentation)

Maxime

Signature · 10 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150114/2bd3c29f/attachment-0001.sig>
author
#4 lore raw

PATCH 1/4 rtc: armada38x: Add the device tree binding documentation

In reply to Arnaud Ebalard

Hi Arnaud,

First, thanks for your extensive review.

Quoted 31 lines · Arnaud Ebalard

Hi Gregory,

Quoted 16 lines · 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 <[email protected]>
---
 .../devicetree/bindings/rtc/armada-380-rtc.txt          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/armada-380-rtc.txt
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +0 -0
new file mode 100644
index 000000000000..2c56bef9dd7e

Documentation/devicetree/bindings/rtc/armada-380-rtc.txt line 8

Quoted 8 lines
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +8 -0
@@ -0,0 +1,17 @@
+* 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. The second entires is for the IP configuration part.

                        ^^^^^^^
                        entry
Quoted 2 lines

I had to read probe function to understand precisely the purpose of each region, i.e. IMHO the description could be made a bit clearer.

Right, and as suggested by Maxime I will use reg-names

Quoted 11 lines
Quoted 11 lines
Documentation/devicetree/bindings/rtc/armada-380-rtc.txt new file +9 -0
+- interrupts: IRQ line for the RTC.
 
+
+Example:
+
+rtc at 184a8 {
+	compatible = "marvell,armada-380-rtc";
+	reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+};
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
author
#5 lore raw

PATCH 2/4 drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

In reply to 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 <[email protected]>
---
 drivers/rtc/Kconfig         |  10 ++
 drivers/rtc/Makefile        |   1 +
 drivers/rtc/rtc-armada38x.c | 305 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 316 insertions(+)
 create mode 100644 drivers/rtc/rtc-armada38x.c
drivers/rtc/Kconfig +10 -0
index f15cddfeb897..de42ebd4b626 100644
@@ -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
drivers/rtc/Makefile +1 -0
index c8ef3e1e6ccd..03fe5629c464 100644
@@ -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
drivers/rtc/rtc-armada38x.c new file +305 -0

305 lines not on this page · raw message

Signature · 1 line
1.9.1
#6 lore raw

PATCH 2/4 drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

Hi Gregory,

Quoted 86 lines · 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 <[email protected]>
---
 drivers/rtc/Kconfig         |  10 ++
 drivers/rtc/Makefile        |   1 +
 drivers/rtc/rtc-armada38x.c | 305 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 316 insertions(+)
 create mode 100644 drivers/rtc/rtc-armada38x.c
drivers/rtc/Kconfig +10 -0
index f15cddfeb897..de42ebd4b626 100644
@@ -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
drivers/rtc/Makefile +1 -0
index c8ef3e1e6ccd..03fe5629c464 100644
@@ -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
drivers/rtc/rtc-armada38x.c new file +32 -0
new file mode 100644
index 000000000000..06202dcf8001
@@ -0,0 +1,305 @@
+/*
+ * RTC driver for the Armada 38x Marvell SoCs
+ *
+ * Copyright (C) 2015 Marvell
+ *
+ * Gregory Clement <[email protected]>
+ *
+ * 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

drivers/rtc/rtc-armada38x.c line 42

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+#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;

out of curiosity, why use a spinlock and not a mutex?

drivers/rtc/rtc-armada38x.c line 54

Quoted 12 lines
drivers/rtc/rtc-armada38x.c new file +12 -0
+	int		    irq;
+};
+
+/*
+ * According to the datasheet, we need to wait for 5us between each
+ * write
+ */
+static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
+{
+	writel(val, rtc->regs + offset);
+	udelay(5);
+}

In the remaining of the driver, you have various direct calls to writel() w/o that 5?s protection, to update/modifiy rtc registers. Is that 5?s trick specific to a given subpart of the registers? In that case, I guess it would help to update the comment.

Quoted 7 lines
drivers/rtc/rtc-armada38x.c new file +7 -0
+
+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);

drivers/rtc/rtc-armada38x.c line 71

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+
+	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);

Bear with me; I don't have access to HW ERRATA.

Are you guaranteed to get a valid value at third read if you got a bad one before, i.e. no need to put a while loop around that workaround?

Additionally, the workaround seems to be related to time setting. Looking at time setting code below, it seems the issue is also created by the fact the writel() call is not under the protection of the spinlock.

drivers/rtc/rtc-armada38x.c line 77

Quoted 6 lines
drivers/rtc/rtc-armada38x.c new file +6 -0
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	rtc_time_to_tm(time_check, tm);
+
+	return 0;

Does the block provide anyway to detect the oscillator was not running, i.e. the value we are reading is not valid?

Quoted 14 lines
drivers/rtc/rtc-armada38x.c new file +12 -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;
+
+	ret = rtc_tm_to_time(tm, &time);
 
 
+
+	if (ret)
+		goto out;

drivers/rtc/rtc-armada38x.c line 99

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+	/*
+	 * Setting the RTC time not always succeeds. According to the
+	 * errata we need to first write on the status register and
+	 * then wait for 1s before writing to the time register to be
+	 * sure that the data will be taken into account.
+	 */
+	writel(0, rtc->regs + RTC_STATUS);
+	ssleep(1);
+
+	writel(time, rtc->regs + RTC_TIME);

I think writel(time + 1, ...) would correct the one second shift you introduce using you ssleep() above.

Additionally, as discussed above, I do not see why you're not protecting the write using you spinlock.

Quoted 14 lines
drivers/rtc/rtc-armada38x.c new file +14 -0
+
+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);

drivers/rtc/rtc-armada38x.c line 123

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+	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;
+}
+

In the two functions below, regarding RTC interrupt activation, do you need a check that a valid IRQ was passed in the .dts and that devm_request_irq() went ok in probe(), i.e. that rtc->irq > 0?

Quoted 31 lines
drivers/rtc/rtc-armada38x.c new file +31 -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)

drivers/rtc/rtc-armada38x.c line 164

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+{
+	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);

I find the following more readable, but it is subjective:

writel(enabled ? RTC_IRQ1_AL_EN : 0, rtc->regs + RTC_IRQ1_CONF);

Quoted 8 lines
drivers/rtc/rtc-armada38x.c new file +8 -0
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	return 0;
+}
+
+static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
+{

drivers/rtc/rtc-armada38x.c line 182

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+	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);
+	val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
+
+	spin_lock(&rtc->lock);
+
+	writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);

Why not putting the readl() above under protection of the spinlock to protect a change that could occur between the readl() and the writel()?

Quoted 54 lines
drivers/rtc/rtc-armada38x.c new file +54 -0
+	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 const 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(pdev, IORESOURCE_MEM, 0);
+	rtc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rtc->regs))
+		return PTR_ERR(rtc->regs);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	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");

drivers/rtc/rtc-armada38x.c line 246

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+		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;
+	}
+	platform_set_drvdata(pdev, rtc);
+
+	device_init_wakeup(&pdev->dev, 1);

if devm_request_irq() returns an error, should device_init_wakeup() be called? See comment below under armada38x_rtc_suspend().

drivers/rtc/rtc-armada38x.c line 253

Quoted 7 lines
drivers/rtc/rtc-armada38x.c new file +7 -0
+	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);
+		if (ret == 0)
+			ret = -EINVAL;

I may be missing something but I do not see how ret can be 0 above because the initial check uses IS_ERR() and not IS_ERR_OR_NULL().

drivers/rtc/rtc-armada38x.c line 266

Quoted 13 lines
drivers/rtc/rtc-armada38x.c new file +13 -0
+		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);
+	}

AFAICT, rtc->irq maybe -1 in the call above. You need to either call device_init_wakeup() when devm_request_irq() succeed or check rtc->irq is valid in the "if" above.

Quoted 39 lines
drivers/rtc/rtc-armada38x.c new file +39 -0
+
+	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 <[email protected]>");
+MODULE_LICENSE("GPL");
author
#7 lore raw

PATCH 2/4 drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

Hi Arnaud,

[...]

Quoted 7 lines

drivers/rtc/rtc-armada38x.c line 42

Quoted 5 lines
drivers/rtc/rtc-armada38x.c new file +5 -0
+struct armada38x_rtc {
+	struct rtc_device   *rtc_dev;
+	void __iomem	    *regs;
+	void __iomem	    *regs_soc;
+	spinlock_t	    lock;

out of curiosity, why use a spinlock and not a mutex?

To be able to use it in the interrupt context.

Quoted 11 lines
Quoted 11 lines
drivers/rtc/rtc-armada38x.c new file +9 -0
+	int		    irq;
+};
+
+/*
+ * According to the datasheet, we need to wait for 5us between each
+ * write
+ */
+static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
+{
Quoted 8 lines

drivers/rtc/rtc-armada38x.c line 54

Quoted 3 lines
drivers/rtc/rtc-armada38x.c new file +3 -0
+	writel(val, rtc->regs + offset);
+	udelay(5);
+}

In the remaining of the driver, you have various direct calls to writel() w/o that 5?s protection, to update/modifiy rtc registers. Is that 5?s trick specific to a given subpart of the registers? In that case, I guess it would help to update the comment.

This direct call were done because there was not a call to write just after. As explained in the comment the 5us is needed only _between_ two consecutive write. I can emphasize it needed.

Quoted 22 lines
Quoted 9 lines
drivers/rtc/rtc-armada38x.c new file +7 -0
+
+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);

drivers/rtc/rtc-armada38x.c line 71

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+
+	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);

Bear with me; I don't have access to HW ERRATA.

Quoted 2 lines

Are you guaranteed to get a valid value at third read if you got a bad one before, i.e. no need to put a while loop around that workaround?

I don't have enough information to answer you, maybe the Marvell designer can answer it. I will ask.

Quoted 6 lines

Additionally, the workaround seems to be related to time setting. Looking at time setting code below, it seems the issue is also created by the fact the writel() call is not under the protection of the spinlock.

Quoted 9 lines

drivers/rtc/rtc-armada38x.c line 77

Quoted 6 lines
drivers/rtc/rtc-armada38x.c new file +6 -0
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	rtc_time_to_tm(time_check, tm);
+
+	return 0;

Does the block provide anyway to detect the oscillator was not running, i.e. the value we are reading is not valid?

I didn't see anything related to this, but here again I will ask.

Quoted 13 lines
Quoted 11 lines
drivers/rtc/rtc-armada38x.c new file +9 -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;
+
+	ret = rtc_tm_to_time(tm, &time);
Quoted 16 lines

drivers/rtc/rtc-armada38x.c line 99

Quoted 13 lines
drivers/rtc/rtc-armada38x.c new file +13 -0
+
+	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 1s before writing to the time register to be
+	 * sure that the data will be taken into account.
+	 */
+	writel(0, rtc->regs + RTC_STATUS);
+	ssleep(1);
+
+	writel(time, rtc->regs + RTC_TIME);

I think writel(time + 1, ...) would correct the one second shift you introduce using you ssleep() above.

I will just move the rtc_tm_to_time-) function here, before the second write.

Quoted 3 lines

Additionally, as discussed above, I do not see why you're not protecting the write using you spinlock.

Initially the spinlock was around the 2 writel, but you can't have sleep in a critical section hold by spinlock so I removed it. However it would be possible to use them around each writel, but I am not sure that it makes sens.

Quoted 22 lines
Quoted 22 lines
drivers/rtc/rtc-armada38x.c new file +20 -0
+
+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);
Quoted 8 lines

drivers/rtc/rtc-armada38x.c line 79

Quoted 4 lines
drivers/rtc/rtc-armada38x.c new file +4 -0
+
+	return 0;
+}
+

In the two functions below, regarding RTC interrupt activation, do you need a check that a valid IRQ was passed in the .dts and that devm_request_irq() went ok in probe(), i.e. that rtc->irq > 0?

You're right is the irq is not valid, I can even removed these 2 functions from armada38x_rtc_ops.

Quoted 61 lines
Quoted 32 lines
drivers/rtc/rtc-armada38x.c new file +31 -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)

drivers/rtc/rtc-armada38x.c line 164

Quoted 10 lines
drivers/rtc/rtc-armada38x.c new file +10 -0
+{
+	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);

I find the following more readable, but it is subjective:

writel(enabled ? RTC_IRQ1_AL_EN : 0, rtc->regs + RTC_IRQ1_CONF);

Quoted 13 lines
drivers/rtc/rtc-armada38x.c new file +13 -0
+
+	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);
Quoted 8 lines

drivers/rtc/rtc-armada38x.c line 182

Quoted 5 lines
drivers/rtc/rtc-armada38x.c new file +5 -0
+	val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
+
+	spin_lock(&rtc->lock);
+
+	writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);

Why not putting the readl() above under protection of the spinlock to protect a change that could occur between the readl() and the writel()?

Indeed it would be better.

Quoted 61 lines
Quoted 61 lines
drivers/rtc/rtc-armada38x.c new file +59 -0
+	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 const 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(pdev, IORESOURCE_MEM, 0);
+	rtc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rtc->regs))
+		return PTR_ERR(rtc->regs);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	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");
Quoted 8 lines

drivers/rtc/rtc-armada38x.c line 246

Quoted 5 lines
drivers/rtc/rtc-armada38x.c new file +5 -0
+		rtc->irq = -1;
+	}
+	platform_set_drvdata(pdev, rtc);
+
+	device_init_wakeup(&pdev->dev, 1);

if devm_request_irq() returns an error, should device_init_wakeup() be called? See comment below under armada38x_rtc_suspend().

We should not called it and also set the set_alarm and alarm_irq_enable operation to NULL.

Quoted 11 lines

drivers/rtc/rtc-armada38x.c line 253

Quoted 8 lines
drivers/rtc/rtc-armada38x.c new file +7 -0
+	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);
+		if (ret == 0)
+			ret = -EINVAL;

I may be missing something but I do not see how ret can be 0 above because the initial check uses IS_ERR() and not IS_ERR_OR_NULL().

Indeed I can remove this. The devm_rtc_device_register won't return NULL.

Quoted 11 lines
Quoted 11 lines
drivers/rtc/rtc-armada38x.c new file +9 -0
+		return ret;
+	}
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int armada38x_rtc_suspend(struct device *dev)
+{
+	if (device_may_wakeup(dev)) {
Quoted 8 lines

drivers/rtc/rtc-armada38x.c line 266

Quoted 4 lines
drivers/rtc/rtc-armada38x.c new file +4 -0
+		struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+
+		return enable_irq_wake(rtc->irq);
+	}

AFAICT, rtc->irq maybe -1 in the call above. You need to either call device_init_wakeup() when devm_request_irq() succeed or check rtc->irq is valid in the "if" above.

I won't call device_init_wakeup() if rtc->irq is -1.

Thanks,

Gregory

Quoted 40 lines
Quoted 40 lines
drivers/rtc/rtc-armada38x.c new file +39 -0
+
+	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 <[email protected]>");
+MODULE_LICENSE("GPL");
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
author
#8 lore raw

PATCH 3/4 MAINTAINERS: Add the RTC driver for the Armada38x

In reply to Gregory CLEMENT

Put it in the mvebu entry.

Signed-off-by Gregory CLEMENT <[email protected]>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)
MAINTAINERS +1 -0
index ddb9ac8d32b3..49ce80ef3894 100644
@@ -1157,6 +1157,7 @@ M:	Sebastian Hesselbarth <[email protected]>
 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 <[email protected]>
Signature · 1 line
1.9.1
#9 lore raw

PATCH 3/4 MAINTAINERS: Add the RTC driver for the Armada38x

Hello.

Quoted 12 lines · Gregory CLEMENT

Put it in the mvebu entry.

Signed-off-by Gregory CLEMENT <[email protected]>
---
  MAINTAINERS | 1 +
  1 file changed, 1 insertion(+)
MAINTAINERS +0 -0
index ddb9ac8d32b3..49ce80ef3894 100644

MAINTAINERS M: Sebastian Hesselbarth <[email protected]> · line 1160

Quoted 4 lines
MAINTAINERS +1 -0
@@ -1157,6 +1157,7 @@ M:	Sebastian Hesselbarth <[email protected]>
  L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
  S:	Maintained
  F:	arch/arm/mach-mvebu/
+F:  drivers/rtc/armada38x-rtc

Please use tab, not spaces after "F:".

WBR, Sergei

author
#10 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm/boot/dts/armada-38x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)
arch/arm/boot/dts/armada-38x.dtsi +6 -0
index 04fe80d101f8..22909add8889 100644
@@ -466,6 +466,12 @@
 				clocks = <&gateclk 4>;
 			};
 
+			rtc at 184a8 {
+				compatible = "marvell,armada-380-rtc";
+				reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
 			sata at a8000 {
 				compatible = "marvell,armada-380-ahci";
 				reg = <0xa8000 0x2000>;
Signature · 1 line
1.9.1
#11 lore raw

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

Hi Gregory,

Quoted 14 lines · 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 <[email protected]>
---
 arch/arm/boot/dts/armada-38x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)
arch/arm/boot/dts/armada-38x.dtsi +0 -0
index 04fe80d101f8..22909add8889 100644

arch/arm/boot/dts/armada-38x.dtsi line 474

Quoted 9 lines
arch/arm/boot/dts/armada-38x.dtsi +6 -0
@@ -466,6 +466,12 @@
 				clocks = <&gateclk 4>;
 			};
 
+			rtc at 184a8 {
+				compatible = "marvell,armada-380-rtc";
+				reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
+

Out of curiosity, why not naming the node rtc at a3800? Main argument:
least expectation principle when reading the .dtsi and expecting increasing addresses. Or did I miss sth else?

Quoted 3 lines
arch/arm/boot/dts/armada-38x.dtsi +0 -0
 			sata at a8000 {
 				compatible = "marvell,armada-380-ahci";
 				reg = <0xa8000 0x2000>;
#12 lore raw

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

Quoted 21 lines · Arnaud Ebalard

Hi Gregory,

Quoted 17 lines · 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 <[email protected]>
---
 arch/arm/boot/dts/armada-38x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)
arch/arm/boot/dts/armada-38x.dtsi +0 -0
index 04fe80d101f8..22909add8889 100644
@@ -466,6 +466,12 @@
 				clocks = <&gateclk 4>;
 			};
Quoted 10 lines

arch/arm/boot/dts/armada-38x.dtsi line 474

Quoted 6 lines
arch/arm/boot/dts/armada-38x.dtsi +6 -0
+			rtc at 184a8 {
+				compatible = "marvell,armada-380-rtc";
+				reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
+

Out of curiosity, why not naming the node rtc at a3800? Main argument:
least expectation principle when reading the .dtsi and expecting increasing addresses. Or did I miss sth else?

Actually, the ePAPR even states that the unit-adress must be the first address in reg (section 2.2.1.1 of the ePAPR 1.1)

Maxime

Signature · 10 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150114/64fb9260/attachment.sig>
author
#13 lore raw

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

In reply to Arnaud Ebalard

Hi Arnaud,

Quoted 21 lines · Arnaud Ebalard

Hi Gregory,

Quoted 17 lines · 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 <[email protected]>
---
 arch/arm/boot/dts/armada-38x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)
arch/arm/boot/dts/armada-38x.dtsi +0 -0
index 04fe80d101f8..22909add8889 100644
@@ -466,6 +466,12 @@
 				clocks = <&gateclk 4>;
 			};
Quoted 10 lines

arch/arm/boot/dts/armada-38x.dtsi line 474

Quoted 6 lines
arch/arm/boot/dts/armada-38x.dtsi +6 -0
+			rtc at 184a8 {
+				compatible = "marvell,armada-380-rtc";
+				reg = <0xa3800 0x20>, <0x184a0 0x0c>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
+

Out of curiosity, why not naming the node rtc at a3800? Main argument:
least expectation principle when reading the .dtsi and expecting increasing addresses. Or did I miss sth else?

It was a mistake, I should have used the 0xa3800 offset to name it because most of the RTC registers are located here.

Thanks,

Gregory

Quoted 5 lines
Quoted 5 lines
arch/arm/boot/dts/armada-38x.dtsi +0 -0
 			sata at a8000 {
 				compatible = "marvell,armada-380-ahci";
 				reg = <0xa8000 0x2000>;
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com