[rtc-linux] Intersil ISL12057 driver

[rtc-linux] Intersil ISL12057 driver

[Alexandre Belloni]

Hi,

I know it has been a while that this driver has been submitted but
Intersil is claiming that ISL12057 is a drop in replacement for ds1337.
I don't think there are any features supported by rtc-isl12057.c
that are not supported by rtc-ds1307.c.  Could you confirm rtc-ds1307
works for you?  I'd like to reduce the number of duplicated drivers.

Thanks!

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: Intersil ISL12057 driver

[Arnaud Ebalard]

Hi Alexandre,

Alexandre Belloni <alexandre.belloni@free-electrons.com> writes:

> I know it has been a while that this driver has been submitted but
> Intersil is claiming that ISL12057 is a drop in replacement for ds1337.
> I don't think there are any features supported by rtc-isl12057.c
> that are not supported by rtc-ds1307.c.  Could you confirm rtc-ds1307
> works for you?  I'd like to reduce the number of duplicated drivers.

Sure. I'll take some time tomorrow evening to test how ds1337 works on
my platforms and report back.

a+

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: Intersil ISL12057 driver

[Arnaud Ebalard]

Hi Alexandre,

arno@natisbad.org (Arnaud Ebalard) writes:

> Alexandre Belloni <alexandre.belloni@free-electrons.com> writes:
>
>> I know it has been a while that this driver has been submitted but
>> Intersil is claiming that ISL12057 is a drop in replacement for ds1337.
>> I don't think there are any features supported by rtc-isl12057.c
>> that are not supported by rtc-ds1307.c.  Could you confirm rtc-ds1307
>> works for you?  I'd like to reduce the number of duplicated drivers.
>
> Sure. I'll take some time tomorrow evening to test how ds1337 works on
> my platforms and report back.

With ds1307 driver compiled in and a simple adjustment to my .dts file
to use "dallas,ds1337" as compatible instead of "isil,isl12057":

	isl12057: isl12057@68 {
		compatible = "dallas,ds1337";
		reg = <0x68>;
		wakeup-source;
	};

the system gets a rtc0 after boot:

[    3.440856] rtc-ds1307 0-0068: SET TIME!
[    3.447093] rtc-ds1307 0-0068: rtc core: registered ds1337 as rtc0
[    3.806492] rtc-ds1307 0-0068: setting system clock to 2016-03-24 20:58:16 UTC (1458853096)

Then, hwclock -s, -r and -w options do work as expected. That's for the
good part. Now, two points that needs to be sorted out before switching
from isl12057 driver to ds1307 one and kill the former:

1) Handling of the century bit

in rtc-ds1307.c seems buggy. After rebooting my system on a kernel with
rtc-isl12057.c driver after the tests, the hardware clock was 100 years 
in the future. In rtc-ds1307.c, the logic is the following:

  set time:

    	/* assume 20YY not 19YY */
	tmp = t->tm_year - 100;
	buf[DS1307_REG_YEAR] = bin2bcd(tmp);

	switch (ds1307->type) {
	case ds_1337:
	case ds_1339:
	case ds_3231:
		buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
		break;

  get time:

        /* assume 20YY not 19YY, and ignore DS1337_BIT_CENTURY */
	t->tm_year = bcd2bin(ds1307->regs[DS1307_REG_YEAR]) + 100;


So, when the driver reads from the rtc, it does not consider the century
bit (it is DS1307_REG_MONTH, notDS1307_REG_YEAR). But when it sets the
time, it blindly sets that bit. This is probably a non-issue because
this will not be seen until before the end of year 2099 ;-). But with
more care, it is possible to honor that century bit and have a driver
that work until 2199. After my hwclock tests with ds1307 driver, the
century bit was detected by isl12057 driver and handled as expected,
resulting in: 

root@sharp:~#  cat /proc/driver/rtc
rtc_time        : 21:28:59
rtc_date        : 2116-03-24
alrm_time       : 20:32:23
alrm_date       : ****-04-24
alarm_IRQ       : yes
alrm_pending    : no
update IRQ enabled      : no
periodic IRQ enabled    : no
periodic IRQ frequency  : 1
max user IRQ frequency  : 64
24hr                    : yes


2) Alarm:

On the devices supported by the kernel that use ISL12057 (Netgear
ReadyNAS 102, 104 and 2120), the interrupt pin of the RTC is not
connected to the SoC but to a PMIC. This means the RTC is capable of
waking up the device when an alarm has been set before the device is 
powered off (power off on evening, wake up in the morning).

But this also means the RTC is a wakeup source but does not have an IRQ
and an associated handler. This is something which is not well supported
by RTC framework (it is not possible to have alarm support w/o IRQ) and
the reason why I introduced the 298ff0122ab19 (rtc: rtc-isl12057: add
isil,irq2-can-wakeup-machine property for in-tree users). I just noticed
(I was not in the recipients when the patch was sent) f4b6722248e49
(rtc: isl12057: enable support for the standard "wakeup-source"
property) changed the property "wakeup-source". I have added the author
(Sudeep Holla) to the recipients; he might have some comments on the
discussion.

With ds1307 driver (and recognized ds1337 type),


	switch (ds1307->type) {
	case ds_1337:
	case ds_1339:
	case ds_3231:

                 <...SNIP...>
                 
		/*
		 * Using IRQ?  Disable the square wave and both alarms.
		 * For some variants, be sure alarms can trigger when we're
		 * running on Vbackup (BBSQI/BBSQW)
		 */
		if (ds1307->client->irq > 0 && chip->alarm) {
			ds1307->regs[0] |= DS1337_BIT_INTCN
					| bbsqi_bitpos[ds1307->type];
			ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);

			want_irq = true;
		}

                 <...SNIP...>


	if (want_irq) {
		device_set_wakeup_capable(&client->dev, true);
		set_bit(HAS_ALARM, &ds1307->flags);
	}

AFAICT, ds1307 driver does not honour wakeup-source property and only
makes its decision based on its ability to register an IRQ handler. For
that reason, the alarm mechanism is no more usable on my platforms:

# cat /sys/class/rtc/rtc0/wakealarm
# echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
-bash: echo: write error: Invalid argument

Cheers,

a+

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: Intersil ISL12057 driver

[Alexandre Belloni]

Hi,

On 24/03/2016 at 23:26:24 +0100, Arnaud Ebalard wrote :
> 1) Handling of the century bit
> 

This indeed needs fixing and it is quite simple. Do you care sending a
patch?

> 2) Alarm:
> 

[...]

Well, my bad, your analysis is correct and I was thinking
8bc2a40730ec74271a0573a6882871308d069f5d was in 4.5 but it will actually
be in 4.6:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8bc2a40730ec74271a0573a6882871308d069f5d

This should solve that one. And you are right, I should probably do
something more generic in the framework to handle "wakeup-source".

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: Intersil ISL12057 driver

[Arnaud Ebalard]

Hi Alexandre,

Alexandre Belloni <alexandre.belloni@free-electrons.com> writes:

> On 24/03/2016 at 23:26:24 +0100, Arnaud Ebalard wrote :
>> 1) Handling of the century bit
>> 
>
> This indeed needs fixing and it is quite simple. Do you care sending a
> patch?

A first version is below. As discussed in the commit message, it is
trivial to make set_ and get_ time functions consistent. But it is not
that easy to extend the capabilities of the driver after 2099 w/o
breaking current platforms.

Basically, the patch below simply remove support for century bit for
ds1307 flavours that have the feature to get things consistent. A sanity
check is also added.

Comments welcome!

>> 2) Alarm:
>> 
>
> [...]
>
> Well, my bad, your analysis is correct and I was thinking
> 8bc2a40730ec74271a0573a6882871308d069f5d was in 4.5 but it will actually
> be in 4.6:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8bc2a40730ec74271a0573a6882871308d069f5d
>
> This should solve that one. And you are right, I should probably do
> something more generic in the framework to handle "wakeup-source".

I have tested a 4.5 with 8bc2a40730ec74 and alarm support works as
expected (i.e. wakeup-source property from dts is honored).

The only minor things to address before removing my (regmap-enabled ;-))
rtc-isl12057.c driver from the tree is the backward compatibility for
"isil,isl12057", "isl,isl12057" and "isil,irq2-can-wakeup-machine" that
may be in existing .dtb and the drivcer support.

Regarding in-kernel users (RN102, RN104 and RN2120), those devices came
with a non-dt capable u-boot; the kernels are ones with an appended dtb,
which is usually updated w/ the kernel. For thoses ones, I do not think
there is any need for retrocompatibility.

Thoughts?

Cheers,

a+


>From 4af5d133daf3a77a39c2f0c17b245ed622e20c16 Mon Sep 17 00:00:00 2001
From: Arnaud Ebalard <arno@natisbad.org>
Date: Fri, 25 Mar 2016 21:21:09 +0100
Subject: [PATCH] rtc: ds1307: fix inconsistent use of century bit

Historically, ds1307 RTC driver mistakenly enabled century
bit when setting the time between 2000 and 2099 (on century
supporting flavour i.e. ds1337, ds1339, ds3231 and ds1340), but
ignored it when reading the time. In practice, this prevents the
driver to support time values after 2099.

This commit removes support for century bit for century capable
flavours supported by the driver to make get and set function
consistent. It also adds an explicit check that passed year
values are in the right range when setting time.

Note that it is not possible to make the driver support time
values after 2099 in a way compatible with currently deployed
versions because RTC chips in the field now have their century
bit set.
---
 drivers/rtc/rtc-ds1307.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index b2156ee5bae1..1537ec83bf8e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -413,20 +413,12 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	buf[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
 
 	/* assume 20YY not 19YY */
+	if (t->tm_year < 100 || t->tm_year > 199)
+		return -EINVAL;
 	tmp = t->tm_year - 100;
 	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
 
 	switch (ds1307->type) {
-	case ds_1337:
-	case ds_1339:
-	case ds_3231:
-		buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
-		break;
-	case ds_1340:
-		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
-				| DS1340_BIT_CENTURY;
-		break;
 	case mcp794xx:
 		/*
 		 * these bits were cleared when preparing the date/time
-- 
2.7.0

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: Intersil ISL12057 driver

[Alexandre Belloni]

On 25/03/2016 at 22:58:40 +0100, Arnaud Ebalard wrote :
> Hi Alexandre,
> 
> Alexandre Belloni <alexandre.belloni@free-electrons.com> writes:
> 
> > On 24/03/2016 at 23:26:24 +0100, Arnaud Ebalard wrote :
> >> 1) Handling of the century bit
> >> 
> >
> > This indeed needs fixing and it is quite simple. Do you care sending a
> > patch?
> 
> A first version is below. As discussed in the commit message, it is
> trivial to make set_ and get_ time functions consistent. But it is not
> that easy to extend the capabilities of the driver after 2099 w/o
> breaking current platforms.
> 
> Basically, the patch below simply remove support for century bit for
> ds1307 flavours that have the feature to get things consistent. A sanity
> check is also added.
> 

Well, we are still a few years away from the cutoff date so we can
probably transition properly. However, some systems (like Android) need
support for date between 1970 and 2000. So I'll need to think a bit more
about it.

> Comments welcome!
> 
> >> 2) Alarm:
> >> 
> >
> > [...]
> >
> > Well, my bad, your analysis is correct and I was thinking
> > 8bc2a40730ec74271a0573a6882871308d069f5d was in 4.5 but it will actually
> > be in 4.6:
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8bc2a40730ec74271a0573a6882871308d069f5d
> >
> > This should solve that one. And you are right, I should probably do
> > something more generic in the framework to handle "wakeup-source".
> 
> I have tested a 4.5 with 8bc2a40730ec74 and alarm support works as
> expected (i.e. wakeup-source property from dts is honored).
> 
> The only minor things to address before removing my (regmap-enabled ;-))

Yeah, your driver is clearly cleaner I waiting to get get some of those
dallas RTCs to clean the ds1307 driver.

> rtc-isl12057.c driver from the tree is the backward compatibility for
> "isil,isl12057", "isl,isl12057" and "isil,irq2-can-wakeup-machine" that
> may be in existing .dtb and the drivcer support.
> 
> Regarding in-kernel users (RN102, RN104 and RN2120), those devices came
> with a non-dt capable u-boot; the kernels are ones with an appended dtb,
> which is usually updated w/ the kernel. For thoses ones, I do not think
> there is any need for retrocompatibility.
> 

Well, if you don't think "isil,irq2-can-wakeup-machine" is needed for
backward compatibility, I'd let it out but I would still add "isl12057" to
the ds1307_id table. This will add support for both compatible strings.

> Thoughts?
> 
> Cheers,
> 
> a+
> 
> 
> From 4af5d133daf3a77a39c2f0c17b245ed622e20c16 Mon Sep 17 00:00:00 2001
> From: Arnaud Ebalard <arno@natisbad.org>
> Date: Fri, 25 Mar 2016 21:21:09 +0100
> Subject: [PATCH] rtc: ds1307: fix inconsistent use of century bit
> 
> Historically, ds1307 RTC driver mistakenly enabled century
> bit when setting the time between 2000 and 2099 (on century
> supporting flavour i.e. ds1337, ds1339, ds3231 and ds1340), but
> ignored it when reading the time. In practice, this prevents the
> driver to support time values after 2099.
> 
> This commit removes support for century bit for century capable
> flavours supported by the driver to make get and set function
> consistent. It also adds an explicit check that passed year
> values are in the right range when setting time.
> 
> Note that it is not possible to make the driver support time
> values after 2099 in a way compatible with currently deployed
> versions because RTC chips in the field now have their century
> bit set.
> ---
>  drivers/rtc/rtc-ds1307.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index b2156ee5bae1..1537ec83bf8e 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -413,20 +413,12 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  	buf[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
>  
>  	/* assume 20YY not 19YY */
> +	if (t->tm_year < 100 || t->tm_year > 199)
> +		return -EINVAL;
>  	tmp = t->tm_year - 100;
>  	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
>  
>  	switch (ds1307->type) {
> -	case ds_1337:
> -	case ds_1339:
> -	case ds_3231:
> -		buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
> -		break;
> -	case ds_1340:
> -		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
> -				| DS1340_BIT_CENTURY;
> -		break;
>  	case mcp794xx:
>  		/*
>  		 * these bits were cleared when preparing the date/time
> -- 
> 2.7.0
> 

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.