[PATCH 1/2] rtc: da9063: set range

[PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

The DA9062 and DA9063 have a year register that can go up to 0x3F.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-da9063.c | 9 ++++++---
 include/linux/rtc.h      | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index 73b38d207d7e..b7052156e851 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c
@@ -464,11 +464,14 @@ static int da9063_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc);
 
-	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, DA9063_DRVNAME_RTC,
-					   &da9063_rtc_ops, THIS_MODULE);
+	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc->rtc_dev))
 		return PTR_ERR(rtc->rtc_dev);
 
+	rtc->rtc_dev->ops = &da9063_rtc_ops;
+	rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	rtc->rtc_dev->range_max = RTC_TIMESTAMP_END_2063;
+
 	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
 	rtc->rtc_sync = false;
 
@@ -481,7 +484,7 @@ static int da9063_rtc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
 			irq_alarm, ret);
 
-	return ret;
+	return rtc_register_device(rtc->rtc_dev);
 }
 
 static struct platform_driver da9063_rtc_driver = {
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 588120ba372c..09fb4af5edab 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -164,6 +164,7 @@ struct rtc_device {
 /* useful timestamps */
 #define RTC_TIMESTAMP_BEGIN_1900	-2208989361LL /* 1900-01-01 00:00:00 */
 #define RTC_TIMESTAMP_BEGIN_2000	946684800LL /* 2000-01-01 00:00:00 */
+#define RTC_TIMESTAMP_END_2063		2966371199LL /* 2063-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_2099		4102444799LL /* 2099-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_9999		253402300799LL /* 9999-12-31 23:59:59 */
 
-- 
2.20.1

[PATCH 2/2] rtc: da9063: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Alexandre Belloni]

Call the 64bit versions of rtc_tm time conversion now that the range is
enforced by the core.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-da9063.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index b7052156e851..1b792bcea3c7 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c
@@ -239,8 +239,8 @@ static int da9063_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 	da9063_data_to_tm(data, tm, rtc);
 
-	rtc_tm_to_time(tm, &tm_secs);
-	rtc_tm_to_time(&rtc->alarm_time, &al_secs);
+	tm_secs = rtc_tm_to_time64(tm);
+	al_secs = rtc_tm_to_time64(&rtc->alarm_time);
 
 	/* handle the rtc synchronisation delay */
 	if (rtc->rtc_sync == true && al_secs - tm_secs == 1)
-- 
2.20.1

RE: [PATCH 2/2] rtc: da9063: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Steve Twiss]

On 21 March 2019 10:16, Alexandre Belloni wrote:

> Subject: [PATCH 2/2] rtc: da9063: switch to rtc_time64_to_tm/rtc_tm_to_time64
> 
> Call the 64bit versions of rtc_tm time conversion now that the range is
> enforced by the core.
> 

Thanks Alexandre,

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

Re: [PATCH 2/2] rtc: da9063: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Thu, Mar 21, 2019 at 11:15:57AM +0100, Alexandre Belloni wrote:
> Call the 64bit versions of rtc_tm time conversion now that the range is
> enforced by the core.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

FWIW, RTC still works on my 32bit system:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] rtc: da9063: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Steve Twiss]

Hi,

On 01 April 2019 09:43, Wolfram Sang wrote:

> Subject: Re: [PATCH 2/2] rtc: da9063: switch to
> rtc_time64_to_tm/rtc_tm_to_time64
> 
> On Thu, Mar 21, 2019 at 11:15:57AM +0100, Alexandre Belloni wrote:
> > Call the 64bit versions of rtc_tm time conversion now that the range is
> > enforced by the core.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> 
> FWIW, RTC still works on my 32bit system:

Thanks!

> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I've also tested the RTC alarm with this patch.
It seems okay to me, no regressions.

Tested-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

Hi Alexandre,

Thanks.

Sorry, I didn't see this immediately because of our e-mail filtering algorithm,

On 21 March 2019 10:16, Alexandre Belloni wrote,

> Subject: [PATCH 1/2] rtc: da9063: set range
> The DA9062 and DA9063 have a year register that can go up to 0x3F.

Yep.

[...]
> index 588120ba372c..09fb4af5edab 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -164,6 +164,7 @@ struct rtc_device {
>  /* useful timestamps */
>  #define RTC_TIMESTAMP_BEGIN_1900	-2208989361LL /* 1900-01-01 00:00:00 */
>  #define RTC_TIMESTAMP_BEGIN_2000	946684800LL /* 2000-01-01 00:00:00 */
> +#define RTC_TIMESTAMP_END_2063		2966371199LL /* 2063-12-31 23:59:59 */

Thanks.
Yes.

Register min and max values are:
2000-01-01 00:00:00
2063-12-31 23:59:59

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Thu, Mar 21, 2019 at 11:15:56AM +0100, Alexandre Belloni wrote:
> The DA9062 and DA9063 have a year register that can go up to 0x3F.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Thanks for this patch! Didn't know about the devm_rtc_device_register()
conversion going on. Glad I learned about it.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I couldn't test the upper limit (DA9063 hooked to a 32bit system here),
but lower limit works and RTC in general works.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Apr 1, 2019 at 10:43 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Mar 21, 2019 at 11:15:56AM +0100, Alexandre Belloni wrote:
> > The DA9062 and DA9063 have a year register that can go up to 0x3F.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> Thanks for this patch! Didn't know about the devm_rtc_device_register()
> conversion going on. Glad I learned about it.
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> I couldn't test the upper limit (DA9063 hooked to a 32bit system here),
> but lower limit works and RTC in general works.

BTW, does the RTC alarm interrupt work for you?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

Hi Geert,

On 01 April 2019 10:00, Geert Uytterhoeven wrote:

> Subject: Re: [PATCH 1/2] rtc: da9063: set range
> 
> Hi Wolfram,
> 
> On Mon, Apr 1, 2019 at 10:43 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > On Thu, Mar 21, 2019 at 11:15:56AM +0100, Alexandre Belloni wrote:
> > > The DA9062 and DA9063 have a year register that can go up to 0x3F.
> > >
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > I couldn't test the upper limit (DA9063 hooked to a 32bit system here),
> > but lower limit works and RTC in general works.
> 
> BTW, does the RTC alarm interrupt work for you?

As far as I can tell, there are no RTC alarm regressions.

I am using an i.MX6Q board and with an unmodified v5.1-rc1 kernel, for the
alarms, I see everything working okay WITHOUT Alexandre's patches ...

Then, WITH Alexandre's patches ...
- https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?h=rtc-next&id=05e4ffeadecaa6f501218504b86a6cec89202bd9
- https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?h=rtc-next&id=da44e0eb7ec6853b5d20faba7dc34c48e4bb35c7
... applied to v5.1-rc1, and with the same set-up,

[    2.027144] da9063 1-0058: Device detected (chip-ID: 0x61, var-ID: 0x60)
[    2.483699] da9063-rtc da9063-rtc: DMA mask not set
[    2.523279] da9063-rtc da9063-rtc: registered as rtc0

Linux test 5.1.0-rc1 #1 SMP Mon Apr 1 13:05:32 BST 2019 armv7l GNU/Linux

[...]

[PASS] Setting the current date and time from the da9063-rtc da9063-rtc as 2000-01-01 00:00:00
[PASS] Setting the alarm date and time from the da9063-rtc da9063-rtc as 2000-01-01 00:00:05 (+5 secs into the future)
[PASS] Setting the listener on da9063-rtc da9063-rtc then waiting for elapsed timeout of 15 seconds...
[PASS] The alarm was triggered on da9063-rtc da9063-rtc within the expected time and the alarm happened at 2000-01-01 00:00:05

[PASS] Setting the current date and time from the da9063-rtc da9063-rtc as 2000-01-01 00:00:00
[PASS] Setting the alarm date and time from the da9063-rtc da9063-rtc as 2000-01-01 00:00:15 (+15 secs into the future)
[PASS] Setting the listener on da9063-rtc da9063-rtc then waiting for elapsed timeout of 25 seconds...
[PASS] The alarm was triggered on da9063-rtc da9063-rtc within the expected time and the alarm happened at 2000-01-01 00:00:15

> cat /proc/interrupts | grep da90
247:          2          0          0          0  gpio-mxc  11 Level     da9063-irq
305:          0          0          2          0  da9063-irq   1 Level     ALARM

I get an identical result.
So as far as I can tell, there are no RTC alarm regressions.

Tested-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

Re: [PATCH 1/2] rtc: da9063: set range

[Geert Uytterhoeven]

Hi Steve,

On Mon, Apr 1, 2019 at 2:39 PM Steve Twiss
<stwiss.opensource@diasemi.com> wrote:
> On 01 April 2019 10:00, Geert Uytterhoeven wrote:
> > Subject: Re: [PATCH 1/2] rtc: da9063: set range
> > On Mon, Apr 1, 2019 at 10:43 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > On Thu, Mar 21, 2019 at 11:15:56AM +0100, Alexandre Belloni wrote:
> > > > The DA9062 and DA9063 have a year register that can go up to 0x3F.
> > > >
> > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > >
> > > I couldn't test the upper limit (DA9063 hooked to a 32bit system here),
> > > but lower limit works and RTC in general works.
> >
> > BTW, does the RTC alarm interrupt work for you?
>
> As far as I can tell, there are no RTC alarm regressions.
>
> I am using an i.MX6Q board and with an unmodified v5.1-rc1 kernel, for the
> alarms, I see everything working okay WITHOUT Alexandre's patches ...

Thanks!

I was just wondering whether they work for Wolfram, who has a similar
but different board than I have. Last time I tried, they didn't work for me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

Hi Geert,

On 01 April 2019 13:42, Geert Uytterhoeven wrote

> Subject: Re: [PATCH 1/2] rtc: da9063: set range
> 
> Hi Steve,
> 
> On Mon, Apr 1, 2019 at 2:39 PM Steve Twiss wrote:
> > As far as I can tell, there are no RTC alarm regressions.
> > I am using an i.MX6Q board and with an unmodified v5.1-rc1 kernel, for the
> > alarms, I see everything working okay [...]
> 
> Thanks!
> 
> I was just wondering whether they work for Wolfram, who has a similar
> but different board than I have. Last time I tried, they didn't work for me.

I think I remember, .. was this it?
https://lore.kernel.org/lkml/6ED8E3B22081A4459DAC7699F3695FB7022179F236@SW-EX-MBX02.diasemi.com/

I remember previously I wasn't able to quickly provide you with a regression
test result after you fixed an IRQ problem that affected the DA9063 RTC
(thanks for your help on that btw).

Regards,
Steve

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]


> I was just wondering whether they work for Wolfram, who has a similar
> but different board than I have. Last time I tried, they didn't work for me.

How did you test?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Apr 1, 2019 at 3:21 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > I was just wondering whether they work for Wolfram, who has a similar
> > but different board than I have. Last time I tried, they didn't work for me.
>
> How did you test?

tools/testing/selftests/rtc/rtctest.c cfr.
https://lore.kernel.org/lkml/CAMuHMdWbV2=LfSWz0rFTGRakZ=2iXKOYdM_Uwaov8-OsJpUgoA@mail.gmail.com/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]

On Mon, Apr 01, 2019 at 03:39:43PM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Mon, Apr 1, 2019 at 3:21 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > I was just wondering whether they work for Wolfram, who has a similar
> > > but different board than I have. Last time I tried, they didn't work for me.
> >
> > How did you test?
> 
> tools/testing/selftests/rtc/rtctest.c cfr.
> https://lore.kernel.org/lkml/CAMuHMdWbV2=LfSWz0rFTGRakZ=2iXKOYdM_Uwaov8-OsJpUgoA@mail.gmail.com/

Hmm, with vanilla v5.1-rc3, I don't even get that far:

[==========] Running 7 tests from 2 test cases.
[ RUN      ] rtc.date_read
rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 04:06:40.
[       OK ] rtc.date_read
[ RUN      ] rtc.uie_read

And here it blocks forever. Any pointers?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

On 01/04/2019 17:07:42+0200, Wolfram Sang wrote:
> On Mon, Apr 01, 2019 at 03:39:43PM +0200, Geert Uytterhoeven wrote:
> > Hi Wolfram,
> > 
> > On Mon, Apr 1, 2019 at 3:21 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > > I was just wondering whether they work for Wolfram, who has a similar
> > > > but different board than I have. Last time I tried, they didn't work for me.
> > >
> > > How did you test?
> > 
> > tools/testing/selftests/rtc/rtctest.c cfr.
> > https://lore.kernel.org/lkml/CAMuHMdWbV2=LfSWz0rFTGRakZ=2iXKOYdM_Uwaov8-OsJpUgoA@mail.gmail.com/
> 
> Hmm, with vanilla v5.1-rc3, I don't even get that far:
> 
> [==========] Running 7 tests from 2 test cases.
> [ RUN      ] rtc.date_read
> rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 04:06:40.
> [       OK ] rtc.date_read
> [ RUN      ] rtc.uie_read
> 
> And here it blocks forever. Any pointers?
>

This means that you don't get any interrupt from your RTC (and that I
have to fix the timeout for uie_read).

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]


> > [==========] Running 7 tests from 2 test cases.
> > [ RUN      ] rtc.date_read
> > rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 04:06:40.
> > [       OK ] rtc.date_read
> > [ RUN      ] rtc.uie_read
> > 
> > And here it blocks forever. Any pointers?
> >
> 
> This means that you don't get any interrupt from your RTC (and that I

Thought so. Well, somehow makes sense that no interrupt gets through,
and not only alarm interrupts...

> have to fix the timeout for uie_read).

:) I am happy to test.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

On 01/04/2019 17:52:04+0200, Wolfram Sang wrote:
> 
> > > [==========] Running 7 tests from 2 test cases.
> > > [ RUN      ] rtc.date_read
> > > rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 04:06:40.
> > > [       OK ] rtc.date_read
> > > [ RUN      ] rtc.uie_read
> > > 
> > > And here it blocks forever. Any pointers?
> > >
> > 
> > This means that you don't get any interrupt from your RTC (and that I
> 
> Thought so. Well, somehow makes sense that no interrupt gets through,
> and not only alarm interrupts...
> 
> > have to fix the timeout for uie_read).
> 
> :) I am happy to test.
> 

Well, seeing the code, I actually remembered that this test is still
there to ensure the core will properly block. If you remove that test,
the other ones should all timeout.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2049 bytes --]


> Well, seeing the code, I actually remembered that this test is still
> there to ensure the core will properly block. If you remove that test,
> the other ones should all timeout.

Thanks for your assistance! What I did just now was to make use of the
'uie_unsupported' flag. This is the outcome:


[==========] Running 7 tests from 2 test cases.
[ RUN      ] rtc.date_read
rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 00:13:23.
[       OK ] rtc.date_read
[ RUN      ] rtc.uie_read
[       OK ] rtc.uie_read
[ RUN      ] rtc.uie_select
[       OK ] rtc.uie_select
[ RUN      ] rtc.alarm_alm_set
rtctest.c:137:rtc.alarm_alm_set:Alarm time now set to 00:13:32.
rtctest.c:148:rtc.alarm_alm_set:Expected 0 (0) != rc (0)
rtc.alarm_alm_set: Test terminated by assertion
[     FAIL ] rtc.alarm_alm_set
[ RUN      ] rtc.alarm_wkalm_set
rtctest.c:195:rtc.alarm_wkalm_set:Alarm time now set to 01/01/2000
00:13:37.
rtctest.c:202:rtc.alarm_wkalm_set:Expected 0 (0) != rc (0)
rtc.alarm_wkalm_set: Test terminated by assertion
[     FAIL ] rtc.alarm_wkalm_set
[ RUN      ] rtc.alarm_alm_set_minute
rtctest.c:239:rtc.alarm_alm_set_minute:Alarm time now set to 00:14:00.
rtctest.c:258:rtc.alarm_alm_set_minute:data: 1a0
[       OK ] rtc.alarm_alm_set_minute
[ RUN      ] rtc.alarm_wkalm_set_minute
rtctest.c:297:rtc.alarm_wkalm_set_minute:Alarm time now set to
01/01/2000 00:15:00.
[       OK ] rtc.alarm_wkalm_set_minute
[==========] 5 / 7 tests passed.
[  FAILED  ]

I wonder why the_set_minute tests pass, but the other ones fail. I also
wonder why I need the uie_unsupported flag? It's been a while since I
dug into the RTC subsystem, I may be missing something. But I see the
UIE code finally calling into set_alarm for some codepath. We have that
for DA9063, but it is not executed for the UIE test of rtctest. However,
it seems the driver doesn't support this in an optimal way, because
there is a currently unused update interrupt which should be used for
UIE, or? I also wonder why all this works fine for Steve.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

On 01/04/2019 21:34:25+0200, Wolfram Sang wrote:
> 
> > Well, seeing the code, I actually remembered that this test is still
> > there to ensure the core will properly block. If you remove that test,
> > the other ones should all timeout.
> 
> Thanks for your assistance! What I did just now was to make use of the
> 'uie_unsupported' flag. This is the outcome:
> 
> 
> [==========] Running 7 tests from 2 test cases.
> [ RUN      ] rtc.date_read
> rtctest.c:49:rtc.date_read:Current RTC date/time is 01/01/2000 00:13:23.
> [       OK ] rtc.date_read
> [ RUN      ] rtc.uie_read
> [       OK ] rtc.uie_read
> [ RUN      ] rtc.uie_select
> [       OK ] rtc.uie_select
> [ RUN      ] rtc.alarm_alm_set
> rtctest.c:137:rtc.alarm_alm_set:Alarm time now set to 00:13:32.
> rtctest.c:148:rtc.alarm_alm_set:Expected 0 (0) != rc (0)
> rtc.alarm_alm_set: Test terminated by assertion
> [     FAIL ] rtc.alarm_alm_set
> [ RUN      ] rtc.alarm_wkalm_set
> rtctest.c:195:rtc.alarm_wkalm_set:Alarm time now set to 01/01/2000
> 00:13:37.
> rtctest.c:202:rtc.alarm_wkalm_set:Expected 0 (0) != rc (0)
> rtc.alarm_wkalm_set: Test terminated by assertion
> [     FAIL ] rtc.alarm_wkalm_set
> [ RUN      ] rtc.alarm_alm_set_minute
> rtctest.c:239:rtc.alarm_alm_set_minute:Alarm time now set to 00:14:00.
> rtctest.c:258:rtc.alarm_alm_set_minute:data: 1a0
> [       OK ] rtc.alarm_alm_set_minute
> [ RUN      ] rtc.alarm_wkalm_set_minute
> rtctest.c:297:rtc.alarm_wkalm_set_minute:Alarm time now set to
> 01/01/2000 00:15:00.
> [       OK ] rtc.alarm_wkalm_set_minute
> [==========] 5 / 7 tests passed.
> [  FAILED  ]
> 
> I wonder why the_set_minute tests pass, but the other ones fail. I also
> wonder why I need the uie_unsupported flag? It's been a while since I
> dug into the RTC subsystem, I may be missing something. But I see the
> UIE code finally calling into set_alarm for some codepath. We have that
> for DA9063, but it is not executed for the UIE test of rtctest. However,
> it seems the driver doesn't support this in an optimal way, because
> there is a currently unused update interrupt which should be used for
> UIE, or? I also wonder why all this works fine for Steve.
> 

I had a look at the driver and I guess you have a 9063AD while Steve
uses another model.

That explains why you need the uie_unsupported flag. The 9063AD can only
do alarms on a minute boundary.

Since the move to hr_timer, the uie are done using the classic alarm or
they are emulated by the core. This improved the situation for many RTCs
that don't have a separate UIE but this made it worse for a few (and
this is an example). I have plan to work on this but didn't have the
time yet.

I suggest the following patch:

===

From 37b2ab7d537e76e42bde64cf4b57701b0ed8e8cd Mon Sep 17 00:00:00 2001
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: Tue, 2 Apr 2019 10:06:46 +0200
Subject: [PATCH] rtc: da9063: set uie_unsupported when relevant

The DA9063AD doesn't support alarms on any seconds and its granularity is
the minute. Set uie_unsupported in that case.

Reported-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-da9063.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index 1b792bcea3c7..53e690b0f3a2 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c
@@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device *pdev)
 	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
 	rtc->rtc_sync = false;
 
+	if (config->rtc_data_start != RTC_SEC)
+		rtc->rtc_dev->uie_unsupported = 1;
+
 	irq_alarm = platform_get_irq_byname(pdev, "ALARM");
 	ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
 					da9063_alarm_event,
-- 
2.20.1


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2580 bytes --]

Hi Alexandre,

> I had a look at the driver and I guess you have a 9063AD while Steve
> uses another model.
> 
> That explains why you need the uie_unsupported flag. The 9063AD can only
> do alarms on a minute boundary.

Bingo! Nice catch. I was on the wrong track because we have an early
boot quirk handling for the DA on this platform and I was searching
there for side effects. Makes all sense now. Thanks a lot for your help!

> Since the move to hr_timer, the uie are done using the classic alarm or
> they are emulated by the core. This improved the situation for many RTCs
> that don't have a separate UIE but this made it worse for a few (and
> this is an example). I have plan to work on this but didn't have the
> time yet.

I understand. That explains why my RTC knowledge from a few years ago
feels so outdated :)

> I suggest the following patch:
> 
> ===
> 
> From 37b2ab7d537e76e42bde64cf4b57701b0ed8e8cd Mon Sep 17 00:00:00 2001
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Date: Tue, 2 Apr 2019 10:06:46 +0200
> Subject: [PATCH] rtc: da9063: set uie_unsupported when relevant
> 
> The DA9063AD doesn't support alarms on any seconds and its granularity is
> the minute. Set uie_unsupported in that case.
> 
> Reported-by: Wolfram Sang <wsa@the-dreams.de>

Please use this address:

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

And probably Geert wants his "+renesas" address, too:

Geert Uytterhoeven <geert+renesas@glider.be>

> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-da9063.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> index 1b792bcea3c7..53e690b0f3a2 100644
> --- a/drivers/rtc/rtc-da9063.c
> +++ b/drivers/rtc/rtc-da9063.c
> @@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device *pdev)
>  	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
>  	rtc->rtc_sync = false;
>  
> +	if (config->rtc_data_start != RTC_SEC)
> +		rtc->rtc_dev->uie_unsupported = 1;
> +

I think we should have a comment here, like:

/* FIXME: Make use of the TICK interrupt once the RTC core supports it */

So, this helps the UIE test:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

And I guess we have to live with two of the alarm tests failing because
of the minute granularity?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

On 02/04/2019 11:33:59+0200, Wolfram Sang wrote:
> Hi Alexandre,
> 
> > I had a look at the driver and I guess you have a 9063AD while Steve
> > uses another model.
> > 
> > That explains why you need the uie_unsupported flag. The 9063AD can only
> > do alarms on a minute boundary.
> 
> Bingo! Nice catch. I was on the wrong track because we have an early
> boot quirk handling for the DA on this platform and I was searching
> there for side effects. Makes all sense now. Thanks a lot for your help!
> 
> > Since the move to hr_timer, the uie are done using the classic alarm or
> > they are emulated by the core. This improved the situation for many RTCs
> > that don't have a separate UIE but this made it worse for a few (and
> > this is an example). I have plan to work on this but didn't have the
> > time yet.
> 
> I understand. That explains why my RTC knowledge from a few years ago
> feels so outdated :)
> 
> > I suggest the following patch:
> > 
> > ===
> > 
> > From 37b2ab7d537e76e42bde64cf4b57701b0ed8e8cd Mon Sep 17 00:00:00 2001
> > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Date: Tue, 2 Apr 2019 10:06:46 +0200
> > Subject: [PATCH] rtc: da9063: set uie_unsupported when relevant
> > 
> > The DA9063AD doesn't support alarms on any seconds and its granularity is
> > the minute. Set uie_unsupported in that case.
> > 
> > Reported-by: Wolfram Sang <wsa@the-dreams.de>
> 
> Please use this address:
> 
> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> And probably Geert wants his "+renesas" address, too:
> 
> Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> >  drivers/rtc/rtc-da9063.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> > index 1b792bcea3c7..53e690b0f3a2 100644
> > --- a/drivers/rtc/rtc-da9063.c
> > +++ b/drivers/rtc/rtc-da9063.c
> > @@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device *pdev)
> >  	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
> >  	rtc->rtc_sync = false;
> >  
> > +	if (config->rtc_data_start != RTC_SEC)
> > +		rtc->rtc_dev->uie_unsupported = 1;
> > +
> 
> I think we should have a comment here, like:
> 
> /* FIXME: Make use of the TICK interrupt once the RTC core supports it */
> 

Well, My plan is to go over all the uie_unsupported users once the
infrastructure is in place but I'll put a comment there.

> So, this helps the UIE test:
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> And I guess we have to live with two of the alarm tests failing because
> of the minute granularity?
> 
> Regards,
> 
>    Wolfram
> 



-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

Hi,

> > >  drivers/rtc/rtc-da9063.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> > > index 1b792bcea3c7..53e690b0f3a2 100644
> > > --- a/drivers/rtc/rtc-da9063.c
> > > +++ b/drivers/rtc/rtc-da9063.c
> > > @@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device *pdev)
> > >  	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
> > >  	rtc->rtc_sync = false;
> > >
> > > +	if (config->rtc_data_start != RTC_SEC)
> > > +		rtc->rtc_dev->uie_unsupported = 1;
> > > +
> >
> > I think we should have a comment here, like:
> > /* FIXME: Make use of the TICK interrupt once the RTC core supports it */

Is this TICK interrupt suggestion to use the DA9063 TICK interrupt to simulate
a second granularity in the AD alarm?

If I remember correctly, the original DA9063 patch set which was for AD silicon
only, and which was sent to LKML before I took over looking at DA9063, used the
DA9063 1-second TICK interrupt to count-down the seconds from the nearest
minute in order to simulate second resolution on the RTC alarm for AD.

... yes. Here it is. The original patch was from Krystian Garbaciak and tried to
support RTC alarms on the AD silicon to a second resolution by counting down
the DA9063 TICK interrupt:

https://marc.info/?l=lm-sensors&m=134613501230005&w=2

However, I dropped that patch completely and wrote a new RTC device driver
because it didn't work in my tests.

The problem was: the TICK interrupt was indistinguishable from the ALARM
interrupt for a wake event and when I tested AD silicon to wake up an Android
device from suspend or power-off using the RTC IRQ, the device woke up on the
ALARM minute (0 seconds), discovered it was not the correct time and immediately
went back to sleep. Then it woke-up and returned back to sleep every TICK IRQ
second until the correct alarm time was reached (up to 59 times!). At which point
it woke up properly.

Regards,
Steve

Re: [PATCH 1/2] rtc: da9063: set range

[Alexandre Belloni]

On 02/04/2019 10:33:37+0000, Steve Twiss wrote:
> Hi,
> 
> > > >  drivers/rtc/rtc-da9063.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> > > > index 1b792bcea3c7..53e690b0f3a2 100644
> > > > --- a/drivers/rtc/rtc-da9063.c
> > > > +++ b/drivers/rtc/rtc-da9063.c
> > > > @@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device *pdev)
> > > >  	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
> > > >  	rtc->rtc_sync = false;
> > > >
> > > > +	if (config->rtc_data_start != RTC_SEC)
> > > > +		rtc->rtc_dev->uie_unsupported = 1;
> > > > +
> > >
> > > I think we should have a comment here, like:
> > > /* FIXME: Make use of the TICK interrupt once the RTC core supports it */
> 
> Is this TICK interrupt suggestion to use the DA9063 TICK interrupt to simulate
> a second granularity in the AD alarm?
> 
> If I remember correctly, the original DA9063 patch set which was for AD silicon
> only, and which was sent to LKML before I took over looking at DA9063, used the
> DA9063 1-second TICK interrupt to count-down the seconds from the nearest
> minute in order to simulate second resolution on the RTC alarm for AD.
> 
> ... yes. Here it is. The original patch was from Krystian Garbaciak and tried to
> support RTC alarms on the AD silicon to a second resolution by counting down
> the DA9063 TICK interrupt:
> 
> https://marc.info/?l=lm-sensors&m=134613501230005&w=2
> 
> However, I dropped that patch completely and wrote a new RTC device driver
> because it didn't work in my tests.
> 
> The problem was: the TICK interrupt was indistinguishable from the ALARM
> interrupt for a wake event and when I tested AD silicon to wake up an Android
> device from suspend or power-off using the RTC IRQ, the device woke up on the
> ALARM minute (0 seconds), discovered it was not the correct time and immediately
> went back to sleep. Then it woke-up and returned back to sleep every TICK IRQ
> second until the correct alarm time was reached (up to 59 times!). At which point
> it woke up properly.
> 

No, the suggestion is to use the TICK interrupt to have a proper UIE
support even if the alarm has a minute granularity. As stated, this is
not yet supported by the core and need some work. Some RTCs have the
following in their set_alarm:

	if (tm->time.tm_sec) {
		time64_t alarm_time = rtc_tm_to_time64(&tm->time);

		alarm_time += 60 - tm->time.tm_sec;
		rtc_time64_to_tm(alarm_time, &tm->time);
	}

But my plan is to actually expose the capability to userspace and the
core so this doesn't have to be handled in the driver.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 150 bytes --]


> But my plan is to actually expose the capability to userspace and the
> core so this doesn't have to be handled in the driver.

I like that plan!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

On 02 April 2019 12:14 Wolfram Sang wrote,
> Subject: Re: [PATCH 1/2] rtc: da9063: set range
> 
> > But my plan is to actually expose the capability to userspace and the
> > core so this doesn't have to be handled in the driver.
> 
> I like that plan!

fwiw, +1

RE: [PATCH 1/2] rtc: da9063: set range

[Steve Twiss]

Hi,

02 April 2019 09:53 Alexandre Belloni, wrote:
> Subject: Re: [PATCH 1/2] rtc: da9063: set range
> On 01/04/2019 21:34:25+0200, Wolfram Sang wrote:
> >
> > Thanks for your assistance! What I did just now was to make use of the
> > 'uie_unsupported' flag. This is the outcome:

[...]

> > I wonder why the_set_minute tests pass, but the other ones fail.
> > [...]
> > I also wonder why all this works fine for Steve.
> >
> 
> I had a look at the driver and I guess you have a 9063AD while Steve
> uses another model.

That would be my immediate guess also.

https://elixir.bootlin.com/linux/v5.1-rc3/source/include/linux/mfd/da9063/core.h#L39

Reading the PMIC register 0x182 and examining the 4 highest bits of that
value will provide the variant code for the DA9063.

> That explains why you need the uie_unsupported flag. The 9063AD can only
> do alarms on a minute boundary.

For AD, alarms only happen to the minute boundary (i.e. alarms to 0 seconds only)
https://elixir.bootlin.com/linux/v5.1-rc3/source/drivers/rtc/rtc-da9063.c#L84

Whereas, BB and greater can alarm to any of the second values 0-59
https://elixir.bootlin.com/linux/v5.1-rc3/source/drivers/rtc/rtc-da9063.c#L113
 
I can confirm that I was only running my previous regression tests with a
BB and CA compliant silicon version. I didn't even think to mention what variant
I was testing with.

[...]

> I suggest the following patch:
> 
> ===
> 
> From 37b2ab7d537e76e42bde64cf4b57701b0ed8e8cd Mon Sep 17 00:00:00 2001
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Date: Tue, 2 Apr 2019 10:06:46 +0200
> Subject: [PATCH] rtc: da9063: set uie_unsupported when relevant
> 
> The DA9063AD doesn't support alarms on any seconds and its granularity is
> the minute. Set uie_unsupported in that case.
> 
> Reported-by: Wolfram Sang <wsa@the-dreams.de>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-da9063.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> index 1b792bcea3c7..53e690b0f3a2 100644
> --- a/drivers/rtc/rtc-da9063.c
> +++ b/drivers/rtc/rtc-da9063.c
> @@ -475,6 +475,9 @@ static int da9063_rtc_probe(struct platform_device
> *pdev)
>  	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
>  	rtc->rtc_sync = false;
> 
> +	if (config->rtc_data_start != RTC_SEC)
> +		rtc->rtc_dev->uie_unsupported = 1;
> +
>  	irq_alarm = platform_get_irq_byname(pdev, "ALARM");
>  	ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
>  					da9063_alarm_event,

Thanks Alexandre,

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Apologies, I am unable to test this on DA9063 AD silicon since I no longer have
that variant.

Regards,
Steve

Re: [PATCH 1/2] rtc: da9063: set range

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]


> Apologies, I am unable to test this on DA9063 AD silicon since I no longer have
> that variant.

Geert and I have boards with these. I am happy to help, and I guess
Geert, too.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]