arm64: Realtek RTD1295 RTC

Based on QNAP's arch/arm/mach-rtk119x/driver/rtk_rtc_drv.c code and mach-rtk119x/driver/dc2vo/fpga/include/mis_reg.h register definitions.

The base year 2014 was observed on all of Zidoo X9S, ProBox2 Ava and Beelink Lake I.

Signed-off-by Andreas F?rber <[email protected]>

---
v1 -> v2:
* Dropped open/release in favor of probe/remove (Alexandre)
* read_time: Reordered register accesses (Alexandre)
* read_time/set_time: Refactored day calculations to avoid time64_t (Alexandre)
* read_time: Retry if seconds change (Alexandre)
* probe: Added missing RTCACR initialization code
* set_time: Fixed year check (off by 1900)
* set_time: Fixed new seconds (off by factor two)
* Cleaned up debug output (Andrew)
* Added spinlocks around register accesses
* Added masks for register fields

 drivers/rtc/Kconfig       |   8 ++
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-rtd119x.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 263 insertions(+)
 create mode 100644 drivers/rtc/rtc-rtd119x.c

drivers/rtc/Kconfig +8 -0

index 22efa21b1d81..d5a46f311ecb 100644
@@ -1765,6 +1765,14 @@ config RTC_DRV_CPCAP
 	   Say y here for CPCAP rtc found on some Motorola phones
 	   and tablets such as Droid 4.
 
+config RTC_DRV_RTD119X
+	bool "Realtek RTD129x RTC"
+	depends on ARCH_REALTEK || COMPILE_TEST
+	default ARCH_REALTEK
+	help
+	  If you say yes here, you get support for the RTD1295 SoC
+	  Real Time Clock.
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME

drivers/rtc/Makefile +1 -0

index acd366b41c85..55a0a5ca45b0 100644
@@ -131,6 +131,7 @@ obj-$(CONFIG_RTC_DRV_RP5C01)	+= rtc-rp5c01.o
 obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
 obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
 obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
+obj-$(CONFIG_RTC_DRV_RTD119X)	+= rtc-rtd119x.o
 obj-$(CONFIG_RTC_DRV_RV3029C2)	+= rtc-rv3029c2.o
 obj-$(CONFIG_RTC_DRV_RV8803)	+= rtc-rv8803.o
 obj-$(CONFIG_RTC_DRV_RX4581)	+= rtc-rx4581.o

drivers/rtc/rtc-rtd119x.c new file +38 -0

new file mode 100644
index 000000000000..27fa68a5af30
@@ -0,0 +1,254 @@
+/*
+ * Realtek RTD129x RTC
+ *
+ * Copyright (c) 2017 Andreas F?rber
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/spinlock.h>
+
+#define RTD_RTCSEC		0x00
+#define RTD_RTCMIN		0x04
+#define RTD_RTCHR		0x08
+#define RTD_RTCDATE1		0x0c
+#define RTD_RTCDATE2		0x10
+#define RTD_RTCACR		0x28
+#define RTD_RTCEN		0x2c
+#define RTD_RTCCR		0x30
+
+#define RTD_RTCSEC_RTCSEC_MASK		0x7f
+
+#define RTD_RTCMIN_RTCMIN_MASK		0x3f
+
+#define RTD_RTCHR_RTCHR_MASK		0x1f
+
+#define RTD_RTCDATE1_RTCDATE1_MASK	0xff
+
+#define RTD_RTCDATE2_RTCDATE2_MASK	0x7f
+
+#define RTD_RTCACR_RTCPWR		BIT(7)
+

drivers/rtc/rtc-rtd119x.c line 48

Quoted 6 lines · Andreas F?rber

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

+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;

Where is this lock initialised? I would expect a call to spin_lock_init() somewhere.

I also wonder what this lock is protecting, which rtc->ops_lock does not protect?

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:

Quoted 10 lines

drivers/rtc/rtc-rtd119x.c line 48

Quoted 6 lines · Andreas F?rber

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

+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;

Where is this lock initialised? I would expect a call to spin_lock_init() somewhere.

Hm, the spinlock in my irq mux series doesn't have that call either; my reset driver did have it. The zero initialization appears to work OK, but you're probably right that it should be there.

Quoted 12 lines

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:

Quoted 10 lines

drivers/rtc/rtc-rtd119x.c line 48

Quoted 6 lines · Andreas F?rber

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

+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;

Where is this lock initialised? I would expect a call to spin_lock_init() somewhere.

Quoted 3 lines

Hm, the spinlock in my irq mux series doesn't have that call either; my reset driver did have it. The zero initialization appears to work OK, but you're probably right that it should be there.

Hi Andreas

I suspect you will have problems if you enable spin lock debug code, like CONFIG_DEBUG_SPINLOCK.

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:

Quoted 10 lines

drivers/rtc/rtc-rtd119x.c line 48

Quoted 6 lines · Andreas F?rber

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

+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;

Where is this lock initialised? I would expect a call to spin_lock_init() somewhere.

Hm, the spinlock in my irq mux series doesn't have that call either; my reset driver did have it. The zero initialization appears to work OK, but you're probably right that it should be there.

Quoted 2 lines

I also wonder what this lock is protecting, which rtc->ops_lock does not protect?

By that same argument we could ask why so many drivers (and mine, too) are calling rtc_valid_tm() when __rtc_read_time() calls it again...

Quoted 3 lines

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
By that same argument we could ask why so many drivers (and mine, too) are calling rtc_valid_tm() when __rtc_read_time() calls it again...

Most of them probably predates the introduction of the check in __rtc_read_time().

drivers/rtc/rtc-rtd119x.c new file +32 -0

+	int tries = 0;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+ while (true) {
+     tm->tm_sec = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
+     tm->tm_min  = readl_relaxed(data->base + RTD_RTCMIN) & RTD_RTCMIN_RTCMIN_MASK;
+     tm->tm_hour = readl_relaxed(data->base + RTD_RTCHR) & RTD_RTCHR_RTCHR_MASK;
+     day  =  readl_relaxed(data->base + RTD_RTCDATE1) & RTD_RTCDATE1_RTCDATE1_MASK;
+     day |= (readl_relaxed(data->base + RTD_RTCDATE2) & RTD_RTCDATE2_RTCDATE2_MASK) << 8;
+     sec  = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
+     tries++;
+
+     if (sec == tm->tm_sec)
+         break;
+
+     if (tries >= 3) {
+         spin_unlock_irqrestore(&data->lock, flags);
+         return -EINVAL;
+     }
+ }
+ if (tries > 1)
+     dev_dbg(dev, "%s: needed %i tries\n", __func__, tries);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ year = data->base_year;
+ while (day >= rtd119x_rtc_year_days(year)) {
+     day -= rtd119x_rtc_year_days(year);
+     year++;
+ }
+ tm->tm_year = year - 1900;

Not much to add, apart from the spinlock issue already spotted by Andrew.

Review

Quoted 5 lines · Andreas F?rber

(quoted diff) +5 -0

+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;

checkpatch complains this should be made unsigned int

Review

Quoted 6 lines

(quoted diff) +6 -0

+	spinlock_t lock;
+};
+
+static inline int rtd119x_rtc_year_days(int year)
+{
+	return rtc_year_days(1, 12, year);

I'm not sure it is worth wrapping rtc_year_days

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+static inline int rtd119x_rtc_year_days(int year)
+{
+	return rtc_year_days(1, 12, year);

I'm not sure it is worth wrapping rtc_year_days

[snip]

Well, I found your rtc_year_days rather confusing and had to play with the arguments until I got it working as expected, so I wanted an inline function (or macro) as abstraction from my three callers.

Quoted 7 lines

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+static inline int rtd119x_rtc_year_days(int year)
+{
+	return rtc_year_days(1, 12, year);

I'm not sure it is worth wrapping rtc_year_days

[snip]

Quoted 3 lines

Well, I found your rtc_year_days rather confusing and had to play with the arguments until I got it working as expected, so I wanted an inline function (or macro) as abstraction from my three callers.

I agree with that. I was wondering why 1st December was being used. I would say this API does not do too well on Rusty's API Design Manifesto.

It does at least get the day/month/year in the right order ;-)

Andrew

Well, I found your rtc_year_days rather confusing and had to play with the arguments until I got it working as expected, so I wanted an inline function (or macro) as abstraction from my three callers.

Sadly the naming is rather confusing as I am looking for the number of days 365..366, whereas your rtc_year_days is meant to return 0..365 and I would just like to extract the 12th array element but need to counter the -1 subtraction. rtc_year_days(31, 11, year) + 1 is not intuitive either - reads like November (and ranges are not documented).

What about exporting a convenient rtc_days_in_year(year) from rtc-lib.c accessing the table directly without rtc_year_days detour? Alternatively an inline function in rtc.h to the same effect without the array?

Also despite is_leap_year() returning a bool || expression you keep using it as array index or integer to add. That assumes true == 1, whereas to my knowledge only false is guaranteed to be 0 and any non-zero value means true. So I'd expect the code to be like this: