[PATCH 0/5] Fix some issues for RTC alarm function

[PATCH 0/5] Fix some issues for RTC alarm function

[Baolin Wang]

This patch set fixes some issues when setting one RTC alarm.

Baolin Wang (5):
  rtc: sc27xx: Set wakeup capability before registering rtc device
  rtc: sc27xx: Clear SPG value update interrupt status
  rtc: sc27xx: Remove interrupts disable and clear in probe()
  rtc: sc27xx: Add check to see if need to enable the alarm interrupt
  rtc: sc27xx: Always read normal alarm when registering RTC device

 drivers/rtc/rtc-sc27xx.c |   60 ++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 20 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] rtc: sc27xx: Set wakeup capability before registering rtc device

[Baolin Wang]

Set wakeup capability before registering rtc device, in case the alarmtimer
can find one available rtc device.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/rtc/rtc-sc27xx.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
index deea5c3..8afba12 100644
--- a/drivers/rtc/rtc-sc27xx.c
+++ b/drivers/rtc/rtc-sc27xx.c
@@ -631,16 +631,18 @@ static int sprd_rtc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	device_init_wakeup(&pdev->dev, 1);
+
 	rtc->rtc->ops = &sprd_rtc_ops;
 	rtc->rtc->range_min = 0;
 	rtc->rtc->range_max = 5662310399LL;
 	ret = rtc_register_device(rtc->rtc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register rtc device\n");
+		device_init_wakeup(&pdev->dev, 0);
 		return ret;
 	}
 
-	device_init_wakeup(&pdev->dev, 1);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 2/5] rtc: sc27xx: Clear SPG value update interrupt status

[Baolin Wang]

We should clear the SPG value update interrupt status once the SPG value
is updated successfully, in case incorrect status validation for next time.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/rtc/rtc-sc27xx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
index 8afba12..6ac5653 100644
--- a/drivers/rtc/rtc-sc27xx.c
+++ b/drivers/rtc/rtc-sc27xx.c
@@ -172,7 +172,8 @@ static int sprd_rtc_lock_alarm(struct sprd_rtc *rtc, bool lock)
 		return ret;
 	}
 
-	return 0;
+	return regmap_write(rtc->regmap, rtc->base + SPRD_RTC_INT_CLR,
+			    SPRD_RTC_SPG_UPD_EN);
 }
 
 static int sprd_rtc_get_secs(struct sprd_rtc *rtc, enum sprd_rtc_reg_types type,
-- 
1.7.9.5

[PATCH 3/5] rtc: sc27xx: Remove interrupts disable and clear in probe()

[Baolin Wang]

When registering one rtc device, it will check to see if there is an
alarm already set in rtc hardware by issuing __rtc_read_alarm(). So
we should not disable the RTC interrupts and clear the interrupts
status in probe() function.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/rtc/rtc-sc27xx.c |   20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
index 6ac5653..4ecabe6 100644
--- a/drivers/rtc/rtc-sc27xx.c
+++ b/drivers/rtc/rtc-sc27xx.c
@@ -129,19 +129,6 @@ static int sprd_rtc_clear_alarm_ints(struct sprd_rtc *rtc)
 			    SPRD_RTC_ALM_INT_MASK);
 }
 
-static int sprd_rtc_disable_ints(struct sprd_rtc *rtc)
-{
-	int ret;
-
-	ret = regmap_update_bits(rtc->regmap, rtc->base + SPRD_RTC_INT_EN,
-				 SPRD_RTC_INT_MASK, 0);
-	if (ret)
-		return ret;
-
-	return regmap_write(rtc->regmap, rtc->base + SPRD_RTC_INT_CLR,
-			    SPRD_RTC_INT_MASK);
-}
-
 static int sprd_rtc_lock_alarm(struct sprd_rtc *rtc, bool lock)
 {
 	int ret;
@@ -609,13 +596,6 @@ static int sprd_rtc_probe(struct platform_device *pdev)
 	rtc->dev = &pdev->dev;
 	platform_set_drvdata(pdev, rtc);
 
-	/* clear all RTC interrupts and disable all RTC interrupts */
-	ret = sprd_rtc_disable_ints(rtc);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to disable RTC interrupts\n");
-		return ret;
-	}
-
 	/* check if RTC time values are valid */
 	ret = sprd_rtc_check_power_down(rtc);
 	if (ret) {
-- 
1.7.9.5

[PATCH 4/5] rtc: sc27xx: Add check to see if need to enable the alarm interrupt

[Baolin Wang]

The RTC interrupt enable register is not put in always-power-on region
supplied by VDDRTC, so we should check if we need enable the alarm
interrupt when system booting.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/rtc/rtc-sc27xx.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
index 4ecabe6..72bb002 100644
--- a/drivers/rtc/rtc-sc27xx.c
+++ b/drivers/rtc/rtc-sc27xx.c
@@ -563,6 +563,32 @@ static int sprd_rtc_check_power_down(struct sprd_rtc *rtc)
 	return 0;
 }
 
+static int sprd_rtc_check_alarm_int(struct sprd_rtc *rtc)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read(rtc->regmap, rtc->base + SPRD_RTC_SPG_VALUE, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * The SPRD_RTC_INT_EN register is not put in always-power-on region
+	 * supplied by VDDRTC, so we should check if we need enable the alarm
+	 * interrupt when system booting.
+	 *
+	 * If we have set SPRD_RTC_POWEROFF_ALM_FLAG which is saved in
+	 * always-power-on region, that means we have set one alarm last time,
+	 * so we should enable the alarm interrupt to help RTC core to see if
+	 * there is an alarm already set in RTC hardware.
+	 */
+	if (!(val & SPRD_RTC_POWEROFF_ALM_FLAG))
+		return 0;
+
+	return regmap_update_bits(rtc->regmap, rtc->base + SPRD_RTC_INT_EN,
+				  SPRD_RTC_ALARM_EN, SPRD_RTC_ALARM_EN);
+}
+
 static int sprd_rtc_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -596,6 +622,13 @@ static int sprd_rtc_probe(struct platform_device *pdev)
 	rtc->dev = &pdev->dev;
 	platform_set_drvdata(pdev, rtc);
 
+	/* check if we need set the alarm interrupt */
+	ret = sprd_rtc_check_alarm_int(rtc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to check RTC alarm interrupt\n");
+		return ret;
+	}
+
 	/* check if RTC time values are valid */
 	ret = sprd_rtc_check_power_down(rtc);
 	if (ret) {
-- 
1.7.9.5

[PATCH 5/5] rtc: sc27xx: Always read normal alarm when registering RTC device

[Baolin Wang]

When registering one RTC device, it will check to see if there is an
alarm already set in RTC hardware by reading RTC alarm, at this time
we should always read the normal alarm put in always-on region by
checking the rtc->registered flag.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/rtc/rtc-sc27xx.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
index 72bb002..b4eb3b3 100644
--- a/drivers/rtc/rtc-sc27xx.c
+++ b/drivers/rtc/rtc-sc27xx.c
@@ -415,10 +415,14 @@ static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u32 val;
 
 	/*
-	 * If aie_timer is enabled, we should get the normal alarm time.
+	 * Before RTC device is registered, it will check to see if there is an
+	 * alarm already set in RTC hardware, and we always read the normal
+	 * alarm at this time.
+	 *
+	 * Or if aie_timer is enabled, we should get the normal alarm time.
 	 * Otherwise we should get auxiliary alarm time.
 	 */
-	if (rtc->rtc && rtc->rtc->aie_timer.enabled == 0)
+	if (rtc->rtc && rtc->rtc->registered && rtc->rtc->aie_timer.enabled == 0)
 		return sprd_rtc_read_aux_alarm(dev, alrm);
 
 	ret = sprd_rtc_get_secs(rtc, SPRD_RTC_ALARM, &secs);
-- 
1.7.9.5

Re: [PATCH 5/5] rtc: sc27xx: Always read normal alarm when registering RTC device

[Alexandre Belloni]

Hello,

On 18/10/2018 16:52:30+0800, Baolin Wang wrote:
> When registering one RTC device, it will check to see if there is an
> alarm already set in RTC hardware by reading RTC alarm, at this time
> we should always read the normal alarm put in always-on region by
> checking the rtc->registered flag.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/rtc/rtc-sc27xx.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
> index 72bb002..b4eb3b3 100644
> --- a/drivers/rtc/rtc-sc27xx.c
> +++ b/drivers/rtc/rtc-sc27xx.c
> @@ -415,10 +415,14 @@ static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	u32 val;
>  
>  	/*
> -	 * If aie_timer is enabled, we should get the normal alarm time.
> +	 * Before RTC device is registered, it will check to see if there is an
> +	 * alarm already set in RTC hardware, and we always read the normal
> +	 * alarm at this time.
> +	 *
> +	 * Or if aie_timer is enabled, we should get the normal alarm time.
>  	 * Otherwise we should get auxiliary alarm time.
>  	 */
> -	if (rtc->rtc && rtc->rtc->aie_timer.enabled == 0)
> +	if (rtc->rtc && rtc->rtc->registered && rtc->rtc->aie_timer.enabled == 0)

Note that the driver should not access rtc->registered and
rtc->aie_timer.enabled and this is a bit fragile.
But, on the other hand, I currently don't have anything better to
suggest. I was also planning to add an in-kernel API for multiple alarms
but I'm not sure it will actually help in your case.

Anyway, this is applied.

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

Re: [PATCH 5/5] rtc: sc27xx: Always read normal alarm when registering RTC device

[Baolin Wang]

Hi Alexandre,

On 25 October 2018 at 08:34, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> Hello,
>
> On 18/10/2018 16:52:30+0800, Baolin Wang wrote:
>> When registering one RTC device, it will check to see if there is an
>> alarm already set in RTC hardware by reading RTC alarm, at this time
>> we should always read the normal alarm put in always-on region by
>> checking the rtc->registered flag.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/rtc/rtc-sc27xx.c |    8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
>> index 72bb002..b4eb3b3 100644
>> --- a/drivers/rtc/rtc-sc27xx.c
>> +++ b/drivers/rtc/rtc-sc27xx.c
>> @@ -415,10 +415,14 @@ static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>       u32 val;
>>
>>       /*
>> -      * If aie_timer is enabled, we should get the normal alarm time.
>> +      * Before RTC device is registered, it will check to see if there is an
>> +      * alarm already set in RTC hardware, and we always read the normal
>> +      * alarm at this time.
>> +      *
>> +      * Or if aie_timer is enabled, we should get the normal alarm time.
>>        * Otherwise we should get auxiliary alarm time.
>>        */
>> -     if (rtc->rtc && rtc->rtc->aie_timer.enabled == 0)
>> +     if (rtc->rtc && rtc->rtc->registered && rtc->rtc->aie_timer.enabled == 0)
>
> Note that the driver should not access rtc->registered and
> rtc->aie_timer.enabled and this is a bit fragile.
> But, on the other hand, I currently don't have anything better to
> suggest. I was also planning to add an in-kernel API for multiple alarms
> but I'm not sure it will actually help in your case.

Yes, I understand your concern. I will glad to help to test if you
introduce new APIs for multiple alarms to see if they can help in our
case. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH 0/5] Fix some issues for RTC alarm function

[Alexandre Belloni]

On 18/10/2018 16:52:25+0800, Baolin Wang wrote:
> This patch set fixes some issues when setting one RTC alarm.
> 
> Baolin Wang (5):
>   rtc: sc27xx: Set wakeup capability before registering rtc device
>   rtc: sc27xx: Clear SPG value update interrupt status
>   rtc: sc27xx: Remove interrupts disable and clear in probe()
>   rtc: sc27xx: Add check to see if need to enable the alarm interrupt
>   rtc: sc27xx: Always read normal alarm when registering RTC device
> 
>  drivers/rtc/rtc-sc27xx.c |   60 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 20 deletions(-)
> 

All applied, thanks.

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