Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720720 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andreas Färber <afaerber@suse.de> - 2017-08-27 02:40 +0200
    Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andrew Lunn <andrew@lunn.ch> - 2017-08-27 04:10 +0200
      Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andreas Färber <afaerber@suse.de> - 2017-08-27 04:40 +0200
        Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andrew Lunn <andrew@lunn.ch> - 2017-08-27 05:30 +0200
          Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andreas Färber <afaerber@suse.de> - 2017-08-27 12:30 +0200
        Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-08-27 10:30 +0200
          Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andreas Färber <afaerber@suse.de> - 2017-08-27 12:30 +0200
    Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-08-27 11:20 +0200
      Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andreas Färber <afaerber@suse.de> - 2017-08-27 13:40 +0200
        Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Andrew Lunn <andrew@lunn.ch> - 2017-08-27 15:40 +0200
          Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-08-27 21:30 +0200
        Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-08-28 18:00 +0200

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

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

[toc] | [next] | [standalone]

#1720728

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

[toc] | [prev] | [next] | [standalone]

#1720730

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)

[toc] | [prev] | [next] | [standalone]

#1720731

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

[toc] | [prev] | [next] | [standalone]

#1720797

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)

[toc] | [prev] | [next] | [standalone]

#1720793

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

[toc] | [prev] | [next] | [standalone]

#1720796

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)

[toc] | [prev] | [next] | [standalone]

#1720795

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

[toc] | [prev] | [next] | [standalone]

#1720802

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)

[toc] | [prev] | [next] | [standalone]

#1720828

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

[toc] | [prev] | [next] | [standalone]

#1720911

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

[toc] | [prev] | [next] | [standalone]

#1721767

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web