[PATCH v2 0/3] arm64: Realtek RTD1295 RTC

[PATCH v2 0/3] arm64: Realtek RTD1295 RTC

[Andreas Färber]

Hello,

This series adds the RTC for the Realtek RTD1295 SoC.
Based on my RTD1295 clk series.

There being no public source code for RTD1295, the implementation is based on
register offsets seen in the vendor DT, as well as older mach-rtk119x code
published by QNAP.

The base year is still hardcoded in v2. Calculations changed.

The DT node depends on the clk series for clock index and header.

More experimental patches at:
https://github.com/afaerber/linux/commits/rtd1295-next

Have a lot of fun!

Cheers,
Andreas

v1 -> v2:
* Updated rtc driver to no longer use open/release (Alexandre)
* Cleaned up debug output (Andrew)
* Avoided COMPILE_TEST division errors (kbuild)
* Various cleanups and extensions

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Cc: linux-rtc@vger.kernel.org
Cc: Roc He <hepeng@zidoo.tv>
Cc: 蒋丽琴 <jiang.liqin@geniatech.com>
Cc: devicetree@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>

Andreas Färber (3):
  dt-bindings: rtc: Add Realtek RTD1295
  rtc: Add Realtek RTD1295
  arm64: dts: realtek: Add RTD1295 RTC node

 .../devicetree/bindings/rtc/realtek,rtd119x.txt    |  16 ++
 arch/arm64/boot/dts/realtek/rtd1295.dtsi           |   6 +
 drivers/rtc/Kconfig                                |   8 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-rtd119x.c                          | 254 +++++++++++++++++++++
 5 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
 create mode 100644 drivers/rtc/rtc-rtd119x.c

-- 
2.12.3

[PATCH v2 1/3] dt-bindings: rtc: Add Realtek RTD1295

[Andreas Färber]

Add a binding for the RTC on the Realtek RTD119x/RTD129x SoC families.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2: Unchanged
 
 .../devicetree/bindings/rtc/realtek,rtd119x.txt          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt

diff --git a/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt b/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
new file mode 100644
index 000000000000..bbf1ccb5df31
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
@@ -0,0 +1,16 @@
+Realtek RTD129x Real-Time Clock
+===============================
+
+Required properties:
+- compatible :  Should be "realtek,rtd1295-rtc"
+- reg        :  Specifies the physical base address and size
+- clocks     :  Specifies the clock gate
+
+
+Example:
+
+	rtc@9801b600 {
+		compatible = "realtek,rtd1295-clk";
+		reg = <0x9801b600 0x100>;
+		clocks = <&clkc RTD1295_CLK_EN_MISC_RTC>;
+	};
-- 
2.12.3

[PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andreas Färber]

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 <afaerber@suse.de>
---
 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

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 22efa21b1d81..d5a46f311ecb 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -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
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index acd366b41c85..55a0a5ca45b0 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -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
diff --git a/drivers/rtc/rtc-rtd119x.c b/drivers/rtc/rtc-rtd119x.c
new file mode 100644
index 000000000000..27fa68a5af30
--- /dev/null
+++ b/drivers/rtc/rtc-rtd119x.c
@@ -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)
+
+#define RTD_RTCEN_RTCEN_MASK		0xff
+
+#define RTD_RTCCR_RTCRST		BIT(6)
+
+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;
+};
+
+static inline int rtd119x_rtc_year_days(int year)
+{
+	return rtc_year_days(1, 12, year);
+}
+
+static void rtd119x_rtc_reset(struct device *dev)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	u32 val;
+
+	val = readl_relaxed(data->base + RTD_RTCCR);
+	val |= RTD_RTCCR_RTCRST;
+	writel_relaxed(val, data->base + RTD_RTCCR);
+
+	val &= ~RTD_RTCCR_RTCRST;
+	writel(val, data->base + RTD_RTCCR);
+}
+
+static void rtd119x_rtc_set_enabled(struct device *dev, bool enable)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	u32 val;
+
+	val = readl_relaxed(data->base + RTD_RTCEN);
+	if (enable) {
+		if ((val & RTD_RTCEN_RTCEN_MASK) == 0x5a)
+			return;
+		writel_relaxed(0x5a, data->base + RTD_RTCEN);
+	} else {
+		writel_relaxed(0, data->base + RTD_RTCEN);
+	}
+}
+
+static int rtd119x_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	unsigned long flags;
+	s32 day;
+	u32 sec;
+	unsigned year;
+	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;
+	tm->tm_yday = day;
+
+	tm->tm_mon = 0;
+	while (day >= rtc_month_days(tm->tm_mon, year)) {
+		day -= rtc_month_days(tm->tm_mon, year);
+		tm->tm_mon++;
+	}
+	tm->tm_mday = day + 1;
+
+	return rtc_valid_tm(tm);
+}
+
+static int rtd119x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	unsigned long flags;
+	unsigned day;
+	int i;
+
+	if (1900 + tm->tm_year < data->base_year)
+		return -EINVAL;
+
+	day = 0;
+	for (i = data->base_year; i < 1900 + tm->tm_year; i++)
+		day += rtd119x_rtc_year_days(i);
+
+	day += tm->tm_yday;
+	if (day > 0x7fff)
+		return -EINVAL;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	rtd119x_rtc_set_enabled(dev, false);
+
+	writel_relaxed((tm->tm_sec << 1) & RTD_RTCSEC_RTCSEC_MASK, data->base + RTD_RTCSEC);
+	writel_relaxed(tm->tm_min & RTD_RTCMIN_RTCMIN_MASK, data->base + RTD_RTCMIN);
+	writel_relaxed(tm->tm_hour & RTD_RTCHR_RTCHR_MASK, data->base + RTD_RTCHR);
+	writel_relaxed(day & RTD_RTCDATE1_RTCDATE1_MASK, data->base + RTD_RTCDATE1);
+	writel_relaxed((day >> 8) & RTD_RTCDATE2_RTCDATE2_MASK, data->base + RTD_RTCDATE2);
+
+	rtd119x_rtc_set_enabled(dev, true);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static const struct rtc_class_ops rtd119x_rtc_ops = {
+	.read_time	= rtd119x_rtc_read_time,
+	.set_time	= rtd119x_rtc_set_time,
+};
+
+static const struct of_device_id rtd119x_rtc_dt_ids[] = {
+	 { .compatible = "realtek,rtd1295-rtc" },
+	 { }
+};
+
+static int rtd119x_rtc_probe(struct platform_device *pdev)
+{
+	struct rtd119x_rtc *data;
+	struct resource *res;
+	u32 val;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+	data->base_year = 2014;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	data->clk = of_clk_get(pdev->dev.of_node, 0);
+	if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		clk_put(data->clk);
+		return ret;
+	}
+
+	val = readl_relaxed(data->base + RTD_RTCACR);
+	if (!(val & RTD_RTCACR_RTCPWR)) {
+		writel_relaxed(RTD_RTCACR_RTCPWR, data->base + RTD_RTCACR);
+
+		rtd119x_rtc_reset(&pdev->dev);
+
+		writel_relaxed(0, data->base + RTD_RTCMIN);
+		writel_relaxed(0, data->base + RTD_RTCHR);
+		writel_relaxed(0, data->base + RTD_RTCDATE1);
+		writel_relaxed(0, data->base + RTD_RTCDATE2);
+	}
+
+	rtd119x_rtc_set_enabled(&pdev->dev, true);
+
+	data->rtcdev = devm_rtc_device_register(&pdev->dev, "rtc",
+				&rtd119x_rtc_ops, THIS_MODULE);
+	if (IS_ERR(data->rtcdev)) {
+		dev_err(&pdev->dev, "failed to register rtc device");
+		clk_disable_unprepare(data->clk);
+		clk_put(data->clk);
+		return PTR_ERR(data->rtcdev);
+	}
+
+	return 0;
+}
+
+static int rtd119x_rtc_remove(struct platform_device *pdev)
+{
+	struct rtd119x_rtc *data = platform_get_drvdata(pdev);
+
+	rtd119x_rtc_set_enabled(&pdev->dev, false);
+
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+static struct platform_driver rtd119x_rtc_driver = {
+	.probe = rtd119x_rtc_probe,
+	.remove = rtd119x_rtc_remove,
+	.driver = {
+		.name = "rtd1295-rtc",
+		.of_match_table	= rtd119x_rtc_dt_ids,
+	},
+};
+builtin_platform_driver(rtd119x_rtc_driver);
-- 
2.12.3

[PATCH v2 3/3] arm64: dts: realtek: Add RTD1295 RTC node

[Andreas Färber]

Add RTC node to the Realtek RTD1295 Device Tree.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2: Unchanged
 
 Depends on the pending clock bindings.
 
 arch/arm64/boot/dts/realtek/rtd1295.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/rtd1295.dtsi b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
index 503e2d5fc334..8ae0949ad89e 100644
--- a/arch/arm64/boot/dts/realtek/rtd1295.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
@@ -192,6 +192,12 @@
 			status = "disabled";
 		};
 
+		rtc@9801b600 {
+			compatible = "realtek,rtd1295-rtc";
+			reg = <0x9801b600 0x100>;
+			clocks = <&clkc RTD1295_CLK_EN_MISC_RTC>;
+		};
+
 		gic: interrupt-controller@ff011000 {
 			compatible = "arm,gic-400";
 			reg = <0xff011000 0x1000>,
-- 
2.12.3

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andrew Lunn]

n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> 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 <afaerber@suse.de>
> ---
>  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
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 22efa21b1d81..d5a46f311ecb 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -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
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index acd366b41c85..55a0a5ca45b0 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -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
> diff --git a/drivers/rtc/rtc-rtd119x.c b/drivers/rtc/rtc-rtd119x.c
> new file mode 100644
> index 000000000000..27fa68a5af30
> --- /dev/null
> +++ b/drivers/rtc/rtc-rtd119x.c
> @@ -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)
> +
> +#define RTD_RTCEN_RTCEN_MASK		0xff
> +
> +#define RTD_RTCCR_RTCRST		BIT(6)
> +
> +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?

    Andrew

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andreas Färber]

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
>> +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.

> 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...

The downstream code is locking inside individual functions such as
check_acr or set_enabled; I adopted it for whole operations only, but
you're right that so far it matches the RTC class ops granularity, so I
can drop it again. The latching discussion had reminded me of locking.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andrew Lunn]

On Sun, Aug 27, 2017 at 04:30:08AM +0200, Andreas Färber wrote:
> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> > n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> >> +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.

Hi Andreas

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

     Andrew

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Alexandre Belloni]

On 27/08/2017 at 04:30:08 +0200, Andreas Färber wrote:
> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> > n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> >> +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.
> 
> > 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...
> 

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


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Alexandre Belloni]

Hi,

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

On 27/08/2017 at 02:33:27 +0200, Andreas Färber wrote:
> +struct rtd119x_rtc {
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct rtc_device *rtcdev;
> +	unsigned base_year;

checkpatch complains this should be made unsigned int

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

> +static int rtd119x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtd119x_rtc *data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	s32 day;
> +	u32 sec;
> +	unsigned year;

unsigned int

> +	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;
> + tm->tm_yday = day;
> +
> + tm->tm_mon = 0;
> + while (day >= rtc_month_days(tm->tm_mon, year)) {
> +     day -= rtc_month_days(tm->tm_mon, year);
> +     tm->tm_mon++;
> + }
> + tm->tm_mday = day + 1;
> +
> + return rtc_valid_tm(tm);

As you spotted, you can return 0 here.

> +}
> +
> +static int rtd119x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtd119x_rtc *data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	unsigned day;

ditto


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andreas Färber]

Hi Andrew,

Am 27.08.2017 um 05:27 schrieb Andrew Lunn:
> On Sun, Aug 27, 2017 at 04:30:08AM +0200, Andreas Färber wrote:
>> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
>>> n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
>>>> +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.
> 
> Hi Andreas
> 
> I suspect you will have problems if you enable spin lock debug code,
> like CONFIG_DEBUG_SPINLOCK.

Thanks for that hint. Surprisingly the irq mux is still fine with that
option enabled, but the rtc now shows:

[    1.193722] BUG: spinlock bad magic on CPU#0, swapper/0/1
[    1.199264]  lock: 0xffff80007bd40db8, .magic: 00000000, .owner:
<none>/-1, .owner_cpu: 0
[    1.207648] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rc6-next-20170825-00061-g88513c6f5687 #180
[    1.217269] Hardware name: Zidoo X9S (DT)
[    1.221378] Call trace:
[    1.223900] [<ffff0000082878f4>] dump_backtrace+0x0/0x320
[    1.229439] [<ffff000008287c28>] show_stack+0x14/0x1c
[    1.234622] [<ffff00000875c6ac>] dump_stack+0x94/0xbc
[    1.239804] [<ffff0000082eba4c>] spin_bug+0x84/0xa4
[    1.244806] [<ffff0000082ebaf8>] do_raw_spin_lock+0x30/0xd4
[    1.250522] [<ffff000008770868>] _raw_spin_lock_irqsave+0x28/0x38
[    1.256772] [<ffff00000864cc14>] rtd119x_rtc_read_time+0x24/0x14c
[    1.263019] [<ffff00000864918c>] __rtc_read_time+0x3c/0x64
[    1.268644] [<ffff0000086491e8>] rtc_read_time+0x34/0x5c
[    1.274091] [<ffff000008649cc8>] __rtc_read_alarm+0x24/0x2f0
[    1.279893] [<ffff000008648b3c>] rtc_device_register+0xa0/0x144
[    1.285962] [<ffff000008648d88>] devm_rtc_device_register+0x5c/0xa4
[    1.292390] [<ffff00000864d038>] rtd119x_rtc_probe+0x174/0x1b4
[    1.298373] [<ffff000008555038>] platform_drv_probe+0x54/0xa8
[    1.304265] [<ffff0000085537dc>] driver_probe_device+0x1fc/0x2b8
[    1.310423] [<ffff00000855390c>] __driver_attach+0x74/0xa4
[    1.316051] [<ffff000008551d58>] bus_for_each_dev+0x80/0x90
[    1.321765] [<ffff000008553198>] driver_attach+0x20/0x28
[    1.327211] [<ffff000008552d8c>] bus_add_driver+0x194/0x1e0
[    1.332925] [<ffff000008554234>] driver_register+0x98/0xd0
[    1.338550] [<ffff000008554f90>] __platform_driver_register+0x48/0x50
[    1.345155] [<ffff0000089924f0>] rtd119x_rtc_driver_init+0x18/0x20
[    1.351493] [<ffff000008283284>] do_one_initcall+0x118/0x13c
[    1.357299] [<ffff000008970dd0>] kernel_init_freeable+0x220/0x224
[    1.363548] [<ffff00000876c128>] kernel_init+0x10/0xf8
[    1.368819] [<ffff000008284200>] ret_from_fork+0x10/0x18
[    1.375104] rtd1295-rtc 9801b600.rtc: rtc core: registered rtc as rtc0

Will fix.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andreas Färber]

Am 27.08.2017 um 10:27 schrieb Alexandre Belloni:
> On 27/08/2017 at 04:30:08 +0200, Andreas Färber wrote:
>> 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().

Replacing with 0 here then.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andreas Färber]

Hi Alexandre,

Am 27.08.2017 um 11:13 schrieb Alexandre Belloni:
> Not much to add, apart from the spinlock issue already spotted by Andrew.
> 
> On 27/08/2017 at 02:33:27 +0200, Andreas Färber wrote:
>> +struct rtd119x_rtc {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	struct rtc_device *rtcdev;
>> +	unsigned base_year;
> 
> checkpatch complains this should be made unsigned int

Ouch, I forgot to add my pre-commit hook in this tree and wasn't aware
of that rule yet. The RFC had it already. Fixed.

>> +	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
[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.

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:

diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c
index 1ae7da5cfc60..8983a408fc30 100644
--- a/drivers/rtc/rtc-lib.c
+++ b/drivers/rtc/rtc-lib.c
@@ -32,7 +32,7 @@ static const unsigned short rtc_ydays[2][13] = {
  */
 int rtc_month_days(unsigned int month, unsigned int year)
 {
-       return rtc_days_in_month[month] + (is_leap_year(year) && month
== 1);
+       return rtc_days_in_month[month] + ((is_leap_year(year) && month
== 1) ? 1 : 0);
 }
 EXPORT_SYMBOL(rtc_month_days);

@@ -41,7 +41,7 @@ EXPORT_SYMBOL(rtc_month_days);
  */
 int rtc_year_days(unsigned int day, unsigned int month, unsigned int year)
 {
-       return rtc_ydays[is_leap_year(year)][month] + day-1;
+       return rtc_ydays[is_leap_year(year) ? 1 : 0][month] + day-1;
 }
 EXPORT_SYMBOL(rtc_year_days);

@@ -69,7 +69,7 @@ void rtc_time64_to_tm(time64_t time, struct rtc_time *tm)
                - LEAPS_THRU_END_OF(1970 - 1);
        if (days < 0) {
                year -= 1;
-               days += 365 + is_leap_year(year);
+               days += 365 + (is_leap_year(year) ? 1 : 0);
        }
        tm->tm_year = year - 1900;
        tm->tm_yday = days + 1;

The above rtc_time64_to_tm() hunk could be converted to the proposed
rtc_days_in_year(). rtc-mcp795.c has another candidate.

By reusing rtc_year_days I elegantly avoided is_leap_year in my code,
but I could spell out 365 + (is_leap_year(year) ? 1 : 0) in my function
if preferred. I dislike duplicating expressions in code.

What do you think?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Andrew Lunn]

> >> +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.

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

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Alexandre Belloni]

On 27/08/2017 at 15:37:51 +0200, Andrew Lunn wrote:
> > >> +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.
> 
> 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 follows the convention that tm_mon is 0-11. Making it easy to read
for anyone used to the RTC subsystem. I can agree it is not the most
intuitive one. That choice predates the RTC subsystem (made in 1996), I
won't give any name ;)

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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295

[Alexandre Belloni]

Hi,

On 27/08/2017 at 13:30:59 +0200, Andreas Färber wrote:
> 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?
> 

This could have been done but what you did in your v3 is fine too. It
will always be time to move that to the core later.

> 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:

sizeof(bool) (actually _Bool) is 1 so it can only be 0 or 1.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com