[PATCH v4 0/4] rtc: OMAP: Add support for rtc-only mode

[PATCH v4 0/4] rtc: OMAP: Add support for rtc-only mode

[Keerthy]

Prepare rtc driver for rtc-only with DDR in self-refresh mode.
The patch series is based on top of Johan Hovald's series:

https://lkml.kernel.org/r/20180704090558.16647-1-johan@kernel.org

Tested for suspend/resume and poweroff on am437x-gp-evm. 

Keerthy (4):
  rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec
  rtc: OMAP: Add support for rtc-only mode
  rtc: omap: use of_device_is_system_power_controller function
  rtc: interface: Add power_off_program to rtc_class_ops

 drivers/rtc/interface.c | 12 +++++++++
 drivers/rtc/rtc-omap.c  | 70 +++++++++++++++++++++++++++++++++++--------------
 include/linux/rtc.h     |  2 ++
 3 files changed, 65 insertions(+), 19 deletions(-)

-- 
1.9.1

[PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Keerthy]

Cut down the shutdown time from 2 seconds to 1 sec. In case of roll
over try again.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

Changes in v4:

  * Fixed a compilation issue.
  * Extended the roll over check post interrupt programming.

 drivers/rtc/rtc-omap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 323ff55..88da927 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -435,17 +435,23 @@ static void omap_rtc_power_off(void)
 	struct rtc_time tm;
 	unsigned long now;
 	u32 val;
+	int seconds;
 
 	rtc->type->unlock(rtc);
 	/* enable pmic_power_en control */
 	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
 	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
 
-	/* set alarm two seconds from now */
+again:
+	/* Clear any existing ALARM2 event */
+	rtc_writel(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM2);
+
+	/* set alarm one second from now */
 	omap_rtc_read_time_raw(rtc, &tm);
+	seconds = tm.tm_sec;
 	bcd2tm(&tm);
 	rtc_tm_to_time(&tm, &now);
-	rtc_time_to_tm(now + 2, &tm);
+	rtc_time_to_tm(now + 1, &tm);
 
 	if (tm2bcd(&tm) < 0) {
 		dev_err(&rtc->rtc->dev, "power off failed\n");
@@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
 	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
 			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
+	/* Our calculations started right before the rollover, try again */
+	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
+		goto again;
 	rtc->type->lock(rtc);
 
 	/*
-- 
1.9.1

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Johan Hovold]

On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
> Cut down the shutdown time from 2 seconds to 1 sec. In case of roll
> over try again.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> 
> Changes in v4:
> 
>   * Fixed a compilation issue.
>   * Extended the roll over check post interrupt programming.
> 
>  drivers/rtc/rtc-omap.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index 323ff55..88da927 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c

First, the comment above this function would need to be updated as part
of this patch as it refers to the two-second alarm offset.

> @@ -435,17 +435,23 @@ static void omap_rtc_power_off(void)
>  	struct rtc_time tm;
>  	unsigned long now;
>  	u32 val;
> +	int seconds;
>  
>  	rtc->type->unlock(rtc);
>  	/* enable pmic_power_en control */
>  	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
>  	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
>  
> -	/* set alarm two seconds from now */
> +again:
> +	/* Clear any existing ALARM2 event */
> +	rtc_writel(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM2);

Why is this needed? Any pending interrupt is cleared at probe, and a
previous attempt to set the alarm really led to the alarm going off, why
would we retry?

> +
> +	/* set alarm one second from now */
>  	omap_rtc_read_time_raw(rtc, &tm);
> +	seconds = tm.tm_sec;
>  	bcd2tm(&tm);
>  	rtc_tm_to_time(&tm, &now);
> -	rtc_time_to_tm(now + 2, &tm);
> +	rtc_time_to_tm(now + 1, &tm);
>  
>  	if (tm2bcd(&tm) < 0) {
>  		dev_err(&rtc->rtc->dev, "power off failed\n");
> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);

Add a newline here.

> +	/* Our calculations started right before the rollover, try again */

Nit: use all lower case unless writing full sentences, which also
matches most of the other comments in this file.

> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
> +		goto again;

Here the alarm may have gone off as part of the roll over, in which case
you shouldn't retry.

Add a newline here too.

>  	rtc->type->lock(rtc);
>  
>  	/*

Thanks,
Johan

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Keerthy]

On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
>> Cut down the shutdown time from 2 seconds to 1 sec. In case of roll
>> over try again.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>
>> Changes in v4:
>>
>>   * Fixed a compilation issue.
>>   * Extended the roll over check post interrupt programming.
>>
>>  drivers/rtc/rtc-omap.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
>> index 323ff55..88da927 100644
>> --- a/drivers/rtc/rtc-omap.c
>> +++ b/drivers/rtc/rtc-omap.c
> 
> First, the comment above this function would need to be updated as part
> of this patch as it refers to the two-second alarm offset.

Yes, will change that.

> 
>> @@ -435,17 +435,23 @@ static void omap_rtc_power_off(void)
>>  	struct rtc_time tm;
>>  	unsigned long now;
>>  	u32 val;
>> +	int seconds;
>>  
>>  	rtc->type->unlock(rtc);
>>  	/* enable pmic_power_en control */
>>  	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
>>  	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
>>  
>> -	/* set alarm two seconds from now */
>> +again:
>> +	/* Clear any existing ALARM2 event */
>> +	rtc_writel(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM2);
> 
> Why is this needed? Any pending interrupt is cleared at probe, and a
> previous attempt to set the alarm really led to the alarm going off, why
> would we retry?

Yes this is not needed.

> 
>> +
>> +	/* set alarm one second from now */
>>  	omap_rtc_read_time_raw(rtc, &tm);
>> +	seconds = tm.tm_sec;
>>  	bcd2tm(&tm);
>>  	rtc_tm_to_time(&tm, &now);
>> -	rtc_time_to_tm(now + 2, &tm);
>> +	rtc_time_to_tm(now + 1, &tm);
>>  
>>  	if (tm2bcd(&tm) < 0) {
>>  		dev_err(&rtc->rtc->dev, "power off failed\n");
>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
> 
> Add a newline here.

Okay

> 
>> +	/* Our calculations started right before the rollover, try again */
> 
> Nit: use all lower case unless writing full sentences, which also
> matches most of the other comments in this file.

okay

> 
>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
>> +		goto again;
> 
> Here the alarm may have gone off as part of the roll over, in which case
> you shouldn't retry.

Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.

In the event of Roll over before setting the
OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
not miss the ALARM2 event? Then poweroff would fail right?

Hence the attempt to retry the next second. This sequence would begin
right at the beginning of a new second and we expect the full sequence
to get over without having to retry again.

Hope i am clear.

> 
> Add a newline here too.

Okay

> 
>>  	rtc->type->lock(rtc);
>>  
>>  	/*
> 
> Thanks,
> Johan
> 

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Keerthy]

On Thursday 19 July 2018 05:23 PM, Keerthy wrote:
> 
> 
> On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
>> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
>>> Cut down the shutdown time from 2 seconds to 1 sec. In case of roll
>>> over try again.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>
>>> Changes in v4:
>>>
>>>   * Fixed a compilation issue.
>>>   * Extended the roll over check post interrupt programming.
>>>
>>>  drivers/rtc/rtc-omap.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
>>> index 323ff55..88da927 100644
>>> --- a/drivers/rtc/rtc-omap.c
>>> +++ b/drivers/rtc/rtc-omap.c
>>
>> First, the comment above this function would need to be updated as part
>> of this patch as it refers to the two-second alarm offset.
> 
> Yes, will change that.
> 
>>
>>> @@ -435,17 +435,23 @@ static void omap_rtc_power_off(void)
>>>  	struct rtc_time tm;
>>>  	unsigned long now;
>>>  	u32 val;
>>> +	int seconds;
>>>  
>>>  	rtc->type->unlock(rtc);
>>>  	/* enable pmic_power_en control */
>>>  	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
>>>  	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
>>>  
>>> -	/* set alarm two seconds from now */
>>> +again:
>>> +	/* Clear any existing ALARM2 event */
>>> +	rtc_writel(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM2);
>>
>> Why is this needed? Any pending interrupt is cleared at probe, and a
>> previous attempt to set the alarm really led to the alarm going off, why
>> would we retry?
> 
> Yes this is not needed.
> 
>>
>>> +
>>> +	/* set alarm one second from now */
>>>  	omap_rtc_read_time_raw(rtc, &tm);
>>> +	seconds = tm.tm_sec;
>>>  	bcd2tm(&tm);
>>>  	rtc_tm_to_time(&tm, &now);
>>> -	rtc_time_to_tm(now + 2, &tm);
>>> +	rtc_time_to_tm(now + 1, &tm);
>>>  
>>>  	if (tm2bcd(&tm) < 0) {
>>>  		dev_err(&rtc->rtc->dev, "power off failed\n");
>>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
>>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
>>
>> Add a newline here.
> 
> Okay
> 
>>
>>> +	/* Our calculations started right before the rollover, try again */
>>
>> Nit: use all lower case unless writing full sentences, which also
>> matches most of the other comments in this file.
> 
> okay
> 
>>
>>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
>>> +		goto again;
>>
>> Here the alarm may have gone off as part of the roll over, in which case
>> you shouldn't retry.
> 
> Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.
> 
> In the event of Roll over before setting the
> OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
> not miss the ALARM2 event? Then poweroff would fail right?
> 
> Hence the attempt to retry the next second. This sequence would begin
> right at the beginning of a new second and we expect the full sequence
> to get over without having to retry again.
> 
> Hope i am clear.

I tried to program the interrupt for the same second on the hardware and
it does not fire. So to take care of roll over corner case one attempt
to retry is needed.

> 
>>
>> Add a newline here too.
> 
> Okay
> 
>>
>>>  	rtc->type->lock(rtc);
>>>  
>>>  	/*
>>
>> Thanks,
>> Johan
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Johan Hovold]

On Thu, Jul 19, 2018 at 05:52:17PM +0530, Keerthy wrote:
> On Thursday 19 July 2018 05:23 PM, Keerthy wrote:
> > On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
> >> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:

> >>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
> >>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
> >>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
> >>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);

> >>> +	/* Our calculations started right before the rollover, try again */

> >>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
> >>> +		goto again;
> >>
> >> Here the alarm may have gone off as part of the roll over, in which case
> >> you shouldn't retry.
> > 
> > Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.
> > 
> > In the event of Roll over before setting the
> > OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
> > not miss the ALARM2 event? Then poweroff would fail right?

Right, that would fail.

> > Hence the attempt to retry the next second. This sequence would begin
> > right at the beginning of a new second and we expect the full sequence
> > to get over without having to retry again.
> > 
> > Hope i am clear.

Yes, sure, but my point is that could end up retrying also after the
alarm has fired correctly (e.g. due to latencies in turning of the
power).

It may be enough to check OMAP_RTC_STATUS_REG before retrying.

> I tried to program the interrupt for the same second on the hardware and
> it does not fire. So to take care of roll over corner case one attempt
> to retry is needed.

Yes, that's expected.

Thanks,
Johan

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Keerthy]

On Thursday 19 July 2018 06:06 PM, Johan Hovold wrote:
> On Thu, Jul 19, 2018 at 05:52:17PM +0530, Keerthy wrote:
>> On Thursday 19 July 2018 05:23 PM, Keerthy wrote:
>>> On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
>>>> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
> 
>>>>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
>>>>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>>>>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>>>>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
> 
>>>>> +	/* Our calculations started right before the rollover, try again */
> 
>>>>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
>>>>> +		goto again;
>>>>
>>>> Here the alarm may have gone off as part of the roll over, in which case
>>>> you shouldn't retry.
>>>
>>> Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.
>>>
>>> In the event of Roll over before setting the
>>> OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
>>> not miss the ALARM2 event? Then poweroff would fail right?
> 
> Right, that would fail.
> 
>>> Hence the attempt to retry the next second. This sequence would begin
>>> right at the beginning of a new second and we expect the full sequence
>>> to get over without having to retry again.
>>>
>>> Hope i am clear.
> 
> Yes, sure, but my point is that could end up retrying also after the
> alarm has fired correctly (e.g. due to latencies in turning of the
> power)>
> It may be enough to check OMAP_RTC_STATUS_REG before retrying.

Ah okay. Yes this makes sense. I will use the status to retry.

> 
>> I tried to program the interrupt for the same second on the hardware and
>> it does not fire. So to take care of roll over corner case one attempt
>> to retry is needed.
> 
> Yes, that's expected.
> 
> Thanks,
> Johan
> 

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Keerthy]

On Thursday 19 July 2018 06:16 PM, Keerthy wrote:
> 
> 
> On Thursday 19 July 2018 06:06 PM, Johan Hovold wrote:
>> On Thu, Jul 19, 2018 at 05:52:17PM +0530, Keerthy wrote:
>>> On Thursday 19 July 2018 05:23 PM, Keerthy wrote:
>>>> On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
>>>>> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
>>
>>>>>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
>>>>>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>>>>>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>>>>>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
>>
>>>>>> +	/* Our calculations started right before the rollover, try again */
>>
>>>>>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
>>>>>> +		goto again;
>>>>>
>>>>> Here the alarm may have gone off as part of the roll over, in which case
>>>>> you shouldn't retry.
>>>>
>>>> Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.
>>>>
>>>> In the event of Roll over before setting the
>>>> OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
>>>> not miss the ALARM2 event? Then poweroff would fail right?
>>
>> Right, that would fail.
>>
>>>> Hence the attempt to retry the next second. This sequence would begin
>>>> right at the beginning of a new second and we expect the full sequence
>>>> to get over without having to retry again.
>>>>
>>>> Hope i am clear.
>>
>> Yes, sure, but my point is that could end up retrying also after the
>> alarm has fired correctly (e.g. due to latencies in turning of the
>> power)>
>> It may be enough to check OMAP_RTC_STATUS_REG before retrying.

On a second thought. Status gets set only after the next second.

if ALARM2 status bit is set that surely means interrupt has fired but if
it is not set then there are 2 possibilities

1) ALARM2 is missed as the roll over happened
2) ALARM2 yet to fire as we are yet to get to the next second.

On the other hand Seconds gives me clear indication if we missed the
interrupt or we are about to get one.

> 
> Ah okay. Yes this makes sense. I will use the status to retry.
> 
>>
>>> I tried to program the interrupt for the same second on the hardware and
>>> it does not fire. So to take care of roll over corner case one attempt
>>> to retry is needed.
>>
>> Yes, that's expected.
>>
>> Thanks,
>> Johan
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Johan Hovold]

On Fri, Jul 20, 2018 at 04:03:20PM +0530, Keerthy wrote:
> 
> 
> On Thursday 19 July 2018 06:16 PM, Keerthy wrote:
> > 
> > 
> > On Thursday 19 July 2018 06:06 PM, Johan Hovold wrote:
> >> On Thu, Jul 19, 2018 at 05:52:17PM +0530, Keerthy wrote:
> >>> On Thursday 19 July 2018 05:23 PM, Keerthy wrote:
> >>>> On Thursday 19 July 2018 03:32 PM, Johan Hovold wrote:
> >>>>> On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
> >>
> >>>>>> @@ -470,6 +476,9 @@ static void omap_rtc_power_off(void)
> >>>>>>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
> >>>>>>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
> >>>>>>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
> >>
> >>>>>> +	/* Our calculations started right before the rollover, try again */
> >>
> >>>>>> +	if (seconds != rtc_read(omap_rtc_power_off_rtc, OMAP_RTC_SECONDS_REG))
> >>>>>> +		goto again;
> >>>>>
> >>>>> Here the alarm may have gone off as part of the roll over, in which case
> >>>>> you shouldn't retry.
> >>>>
> >>>> Ex: We programmed at Sec = 2 and we expect ALARM2 to fire at sec = 3.
> >>>>
> >>>> In the event of Roll over before setting the
> >>>> OMAP_RTC_INTERRUPTS_IT_ALARM2 bit in the OMAP_RTC_INTERRUPTS_REG will we
> >>>> not miss the ALARM2 event? Then poweroff would fail right?
> >>
> >> Right, that would fail.
> >>
> >>>> Hence the attempt to retry the next second. This sequence would begin
> >>>> right at the beginning of a new second and we expect the full sequence
> >>>> to get over without having to retry again.
> >>>>
> >>>> Hope i am clear.
> >>
> >> Yes, sure, but my point is that could end up retrying also after the
> >> alarm has fired correctly (e.g. due to latencies in turning of the
> >> power)>
> >> It may be enough to check OMAP_RTC_STATUS_REG before retrying.
> 
> On a second thought. Status gets set only after the next second.
> 
> if ALARM2 status bit is set that surely means interrupt has fired but if
> it is not set then there are 2 possibilities
> 
> 1) ALARM2 is missed as the roll over happened
> 2) ALARM2 yet to fire as we are yet to get to the next second.
> 
> On the other hand Seconds gives me clear indication if we missed the
> interrupt or we are about to get one.

Yes, you still have to check seconds *before* retrying based on status.

That should do, right?

Johan

Re: [PATCH v4 1/4] rtc: omap: Cut down the shutdown time from 2 seconds to 1 sec

[Johan Hovold]

On Thu, Jul 12, 2018 at 10:37:37AM +0530, Keerthy wrote:
> Cut down the shutdown time from 2 seconds to 1 sec. In case of roll
> over try again.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>

> @@ -435,17 +435,23 @@ static void omap_rtc_power_off(void)

> +	/* set alarm one second from now */
>  	omap_rtc_read_time_raw(rtc, &tm);
> +	seconds = tm.tm_sec;
>  	bcd2tm(&tm);
>  	rtc_tm_to_time(&tm, &now);
> -	rtc_time_to_tm(now + 2, &tm);
> +	rtc_time_to_tm(now + 1, &tm);

One more thing, the comment before final mdelay as well as that delay
itself also needs to be updated to reflect this change.

Thanks,
Johan

[PATCH v4 2/4] rtc: OMAP: Add support for rtc-only mode

[Keerthy]

Prepare rtc driver for rtc-only with DDR in self-refresh mode.
omap_rtc_power_off now should cater to two features:

1) RTC plus DDR in self-refresh is power a saving mode where in the
entire system including the different voltage rails from PMIC are
shutdown except the ones feeding on to RTC and DDR. DDR is kept in
self-refresh hence the contents are preserved. RTC ALARM2 is connected
to PMIC_EN line once we the ALARM2 is triggered we enter the mode with
DDR in self-refresh and RTC Ticking. After a predetermined time an RTC
ALARM1 triggers waking up the system[1]. The control goes to bootloader.
The bootloader then checks RTC scratchpad registers to confirm it was an
rtc_only wakeup and follows a different path, configure bare minimal
clocks for ddr and then jumps to the resume address in another RTC
scratchpad registers and transfers the control to Kernel. Kernel then
restores the saved context. omap_rtc_power_off_program does the ALARM2
programming part.

     [1] http://www.ti.com/lit/ug/spruhl7h/spruhl7h.pdf Page 2884

2) Power-off: This is usual poweroff mode. omap_rtc_power_off calls the
above omap_rtc_power_off_program function and in addition to that
programs the OMAP_RTC_PMIC_REG for any external wake ups for PMIC like
the pushbutton and shuts off the PMIC.

Hence the split in omap_rtc_power_off.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/rtc/rtc-omap.c | 53 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 88da927..cb19ece 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -415,21 +415,12 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 
 static struct omap_rtc *omap_rtc_power_off_rtc;
 
-/*
- * omap_rtc_poweroff: RTC-controlled power off
- *
- * The RTC can be used to control an external PMIC via the pmic_power_en pin,
- * which can be configured to transition to OFF on ALARM2 events.
- *
- * Notes:
- * The two-second alarm offset is the shortest offset possible as the alarm
- * registers must be set before the next timer update and the offset
- * calculation is too heavy for everything to be done within a single access
- * period (~15 us).
- *
- * Called with local interrupts disabled.
+/**
+ * omap_rtc_power_off_program: Set the pmic power off sequence. The RTC
+ * generates pmic_pwr_enable control, which can be used to control an external
+ * PMIC.
  */
-static void omap_rtc_power_off(void)
+static int omap_rtc_power_off_program(struct device *dev)
 {
 	struct omap_rtc *rtc = omap_rtc_power_off_rtc;
 	struct rtc_time tm;
@@ -456,7 +447,7 @@ static void omap_rtc_power_off(void)
 	if (tm2bcd(&tm) < 0) {
 		dev_err(&rtc->rtc->dev, "power off failed\n");
 		rtc->type->lock(rtc);
-		return;
+		return -EINVAL;
 	}
 
 	rtc_wait_not_busy(rtc);
@@ -481,6 +472,38 @@ static void omap_rtc_power_off(void)
 		goto again;
 	rtc->type->lock(rtc);
 
+	return 0;
+}
+
+/*
+ * omap_rtc_poweroff: RTC-controlled power off
+ *
+ * The RTC can be used to control an external PMIC via the pmic_power_en pin,
+ * which can be configured to transition to OFF on ALARM2 events.
+ *
+ * Notes:
+ * The one-second alarm offset is the shortest offset possible as the alarm
+ * registers must be set before the next timer update and the offset
+ * calculation is too heavy for everything to be done within a single access
+ * period (~15 us).
+ *
+ * Called with local interrupts disabled.
+ */
+static void omap_rtc_power_off(void)
+{
+	struct rtc_device *rtc = omap_rtc_power_off_rtc->rtc;
+	u32 val;
+
+	omap_rtc_power_off_program(rtc->dev.parent);
+
+	/* Set PMIC power enable and EXT_WAKEUP in case PB power on is used */
+	omap_rtc_power_off_rtc->type->unlock(omap_rtc_power_off_rtc);
+	val = rtc_readl(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG);
+	val |= OMAP_RTC_PMIC_POWER_EN_EN | OMAP_RTC_PMIC_EXT_WKUP_POL(0) |
+			OMAP_RTC_PMIC_EXT_WKUP_EN(0);
+	rtc_writel(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG, val);
+	omap_rtc_power_off_rtc->type->lock(omap_rtc_power_off_rtc);
+
 	/*
 	 * Wait for alarm to trigger (within two seconds) and external PMIC to
 	 * power off the system. Add a 500 ms margin for external latencies
-- 
1.9.1

Re: [PATCH v4 2/4] rtc: OMAP: Add support for rtc-only mode

[Johan Hovold]

On Thu, Jul 12, 2018 at 10:37:38AM +0530, Keerthy wrote:
> Prepare rtc driver for rtc-only with DDR in self-refresh mode.
> omap_rtc_power_off now should cater to two features:
> 
> 1) RTC plus DDR in self-refresh is power a saving mode where in the
> entire system including the different voltage rails from PMIC are
> shutdown except the ones feeding on to RTC and DDR. DDR is kept in
> self-refresh hence the contents are preserved. RTC ALARM2 is connected
> to PMIC_EN line once we the ALARM2 is triggered we enter the mode with
> DDR in self-refresh and RTC Ticking. After a predetermined time an RTC
> ALARM1 triggers waking up the system[1]. The control goes to bootloader.
> The bootloader then checks RTC scratchpad registers to confirm it was an
> rtc_only wakeup and follows a different path, configure bare minimal
> clocks for ddr and then jumps to the resume address in another RTC
> scratchpad registers and transfers the control to Kernel. Kernel then
> restores the saved context. omap_rtc_power_off_program does the ALARM2
> programming part.
> 
>      [1] http://www.ti.com/lit/ug/spruhl7h/spruhl7h.pdf Page 2884
> 
> 2) Power-off: This is usual poweroff mode. omap_rtc_power_off calls the
> above omap_rtc_power_off_program function and in addition to that
> programs the OMAP_RTC_PMIC_REG for any external wake ups for PMIC like
> the pushbutton and shuts off the PMIC.
> 
> Hence the split in omap_rtc_power_off.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/rtc/rtc-omap.c | 53 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index 88da927..cb19ece 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -415,21 +415,12 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  
>  static struct omap_rtc *omap_rtc_power_off_rtc;
>  
> -/*
> - * omap_rtc_poweroff: RTC-controlled power off
> - *
> - * The RTC can be used to control an external PMIC via the pmic_power_en pin,
> - * which can be configured to transition to OFF on ALARM2 events.
> - *
> - * Notes:
> - * The two-second alarm offset is the shortest offset possible as the alarm
> - * registers must be set before the next timer update and the offset
> - * calculation is too heavy for everything to be done within a single access
> - * period (~15 us).
> - *
> - * Called with local interrupts disabled.
> +/**
> + * omap_rtc_power_off_program: Set the pmic power off sequence. The RTC
> + * generates pmic_pwr_enable control, which can be used to control an external
> + * PMIC.

Since this is kerneldoc, you need an empty line after "sequence." above.

You should also add a comment that this function depends on local
interrupts being disabled when called (for the wait_not_busy handling); 
and make sure you follow that in subsequent patches.

>   */
> -static void omap_rtc_power_off(void)
> +static int omap_rtc_power_off_program(struct device *dev)

dev is never used in this function.

>  {
>  	struct omap_rtc *rtc = omap_rtc_power_off_rtc;
>  	struct rtc_time tm;
> @@ -456,7 +447,7 @@ static void omap_rtc_power_off(void)
>  	if (tm2bcd(&tm) < 0) {
>  		dev_err(&rtc->rtc->dev, "power off failed\n");
>  		rtc->type->lock(rtc);
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	rtc_wait_not_busy(rtc);
> @@ -481,6 +472,38 @@ static void omap_rtc_power_off(void)
>  		goto again;
>  	rtc->type->lock(rtc);
>  
> +	return 0;
> +}
> +
> +/*
> + * omap_rtc_poweroff: RTC-controlled power off
> + *
> + * The RTC can be used to control an external PMIC via the pmic_power_en pin,
> + * which can be configured to transition to OFF on ALARM2 events.
> + *
> + * Notes:
> + * The one-second alarm offset is the shortest offset possible as the alarm
> + * registers must be set before the next timer update and the offset
> + * calculation is too heavy for everything to be done within a single access
> + * period (~15 us).

This note really doesn't make much sense anymore, and should have been
updated as part of the previous patch.

> + *
> + * Called with local interrupts disabled.
> + */
> +static void omap_rtc_power_off(void)
> +{
> +	struct rtc_device *rtc = omap_rtc_power_off_rtc->rtc;

Use struct omap_rtc here as before.

> +	u32 val;
> +
> +	omap_rtc_power_off_program(rtc->dev.parent);
> +
> +	/* Set PMIC power enable and EXT_WAKEUP in case PB power on is used */
> +	omap_rtc_power_off_rtc->type->unlock(omap_rtc_power_off_rtc);
> +	val = rtc_readl(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG);
> +	val |= OMAP_RTC_PMIC_POWER_EN_EN | OMAP_RTC_PMIC_EXT_WKUP_POL(0) |
> +			OMAP_RTC_PMIC_EXT_WKUP_EN(0);
> +	rtc_writel(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG, val);
> +	omap_rtc_power_off_rtc->type->lock(omap_rtc_power_off_rtc);

What is all this, and why is it here?

Surely fiddling with this register after you've set the alarm to trigger
isn't the right thing to do.

> +
>  	/*
>  	 * Wait for alarm to trigger (within two seconds) and external PMIC to
>  	 * power off the system. Add a 500 ms margin for external latencies

Johan

[PATCH v4 3/4] rtc: omap: use of_device_is_system_power_controller function

[Keerthy]

Use of_device_is_system_power_controller instead of manually reading
the system-power-controller property from the device tree node.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/rtc/rtc-omap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index cb19ece..3610efd 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -753,8 +753,7 @@ static int omap_rtc_probe(struct platform_device *pdev)
 	if (of_id) {
 		rtc->type = of_id->data;
 		rtc->is_pmic_controller = rtc->type->has_pmic_mode &&
-				of_property_read_bool(pdev->dev.of_node,
-						"system-power-controller");
+			of_device_is_system_power_controller(pdev->dev.of_node);
 	} else {
 		id_entry = platform_get_device_id(pdev);
 		rtc->type = (void *)id_entry->driver_data;
-- 
1.9.1

Re: [PATCH v4 3/4] rtc: omap: use of_device_is_system_power_controller function

[Johan Hovold]

On Thu, Jul 12, 2018 at 10:37:39AM +0530, Keerthy wrote:
> Use of_device_is_system_power_controller instead of manually reading
> the system-power-controller property from the device tree node.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/rtc/rtc-omap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index cb19ece..3610efd 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -753,8 +753,7 @@ static int omap_rtc_probe(struct platform_device *pdev)
>  	if (of_id) {
>  		rtc->type = of_id->data;
>  		rtc->is_pmic_controller = rtc->type->has_pmic_mode &&
> -				of_property_read_bool(pdev->dev.of_node,
> -						"system-power-controller");
> +			of_device_is_system_power_controller(pdev->dev.of_node);

Nit: Continuation lines should be indented at least two tabs further,
but in this case an exception seems warranted to keep the line within 80
cols.

>  	} else {
>  		id_entry = platform_get_device_id(pdev);
>  		rtc->type = (void *)id_entry->driver_data;

Reviewed-by: Johan Hovold <johan@kernel.org>

[PATCH v4 4/4] rtc: interface: Add power_off_program to rtc_class_ops

[Keerthy]

Add an interface function to set up the rtc for a power_off
mode.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/rtc/interface.c | 12 ++++++++++++
 drivers/rtc/rtc-omap.c  |  1 +
 include/linux/rtc.h     |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 6d4012d..c19668b9 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -1139,3 +1139,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset)
 	trace_rtc_set_offset(offset, ret);
 	return ret;
 }
+
+/**
+ * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN
+ * line and can be used to power off the SoC.
+ *
+ * Kernel interface to program rtc to power off
+ */
+int rtc_power_off_program(struct rtc_device *rtc)
+{
+	return rtc->ops->power_off_program(rtc->dev.parent);
+}
+EXPORT_SYMBOL_GPL(rtc_power_off_program);
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 3610efd..9c9ea44 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -518,6 +518,7 @@ static void omap_rtc_power_off(void)
 	.read_alarm	= omap_rtc_read_alarm,
 	.set_alarm	= omap_rtc_set_alarm,
 	.alarm_irq_enable = omap_rtc_alarm_irq_enable,
+	.power_off_program = omap_rtc_power_off_program,
 };
 
 static const struct omap_rtc_device_type omap_rtc_default_type = {
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 6268208..3fc640c 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -85,6 +85,7 @@ struct rtc_class_ops {
 	int (*alarm_irq_enable)(struct device *, unsigned int enabled);
 	int (*read_offset)(struct device *, long *offset);
 	int (*set_offset)(struct device *, long offset);
+	int (*power_off_program)(struct device *dev);
 };
 
 typedef struct rtc_task {
@@ -229,6 +230,7 @@ int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer *timer,
 int rtc_read_offset(struct rtc_device *rtc, long *offset);
 int rtc_set_offset(struct rtc_device *rtc, long offset);
 void rtc_timer_do_work(struct work_struct *work);
+int rtc_power_off_program(struct rtc_device *rtc);
 
 static inline bool is_leap_year(unsigned int year)
 {
-- 
1.9.1

Re: [PATCH v4 4/4] rtc: interface: Add power_off_program to rtc_class_ops

[Johan Hovold]

On Thu, Jul 12, 2018 at 10:37:40AM +0530, Keerthy wrote:
> Add an interface function to set up the rtc for a power_off
> mode.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/rtc/interface.c | 12 ++++++++++++
>  drivers/rtc/rtc-omap.c  |  1 +
>  include/linux/rtc.h     |  2 ++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 6d4012d..c19668b9 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -1139,3 +1139,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset)
>  	trace_rtc_set_offset(offset, ret);
>  	return ret;
>  }
> +
> +/**
> + * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN
> + * line and can be used to power off the SoC.
> + *
> + * Kernel interface to program rtc to power off
> + */
> +int rtc_power_off_program(struct rtc_device *rtc)
> +{
> +	return rtc->ops->power_off_program(rtc->dev.parent);

Why pass in parent instead of the rtc?

> +}
> +EXPORT_SYMBOL_GPL(rtc_power_off_program);

Either way, this is likely not an acceptable interface for this as
Alexandre already mentioned.

As I already suggested, I think you submit this as part of the PM work
adding support for entering the TI RTC-only mode. That will hopefully
provide enough context to be able to determine the right interface.

Note that this also means that we shouldn't split the current rtc-omap
power-off handler before this has been settled (i.e. patches 2/4 and 4/4
should not be applied before then).

Thanks,
Johan