diff mbox series

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

Message ID 20170827003328.28370-3-afaerber@suse.de
State Superseded
Headers show
Series arm64: Realtek RTD1295 RTC | expand

Commit Message

Andreas Färber Aug. 27, 2017, 12:33 a.m. UTC
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

-- 
2.12.3

Comments

Andrew Lunn Aug. 27, 2017, 2:05 a.m. UTC | #1
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
Andreas Färber Aug. 27, 2017, 2:30 a.m. UTC | #2
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)
Andrew Lunn Aug. 27, 2017, 3:27 a.m. UTC | #3
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
Alexandre Belloni Aug. 27, 2017, 8:27 a.m. UTC | #4
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
Alexandre Belloni Aug. 27, 2017, 9:13 a.m. UTC | #5
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
Andreas Färber Aug. 27, 2017, 10:21 a.m. UTC | #6
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)
Andreas Färber Aug. 27, 2017, 10:28 a.m. UTC | #7
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)
Andreas Färber Aug. 27, 2017, 11:30 a.m. UTC | #8
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)
Andrew Lunn Aug. 27, 2017, 1:37 p.m. UTC | #9
> >> +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
Alexandre Belloni Aug. 27, 2017, 7:26 p.m. UTC | #10
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
Alexandre Belloni Aug. 28, 2017, 3:50 p.m. UTC | #11
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
diff mbox series

Patch

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);