Fix some issues for RTC alarm function

PATCH In review Baolin Wang 2 participants 9 messages Oct 18, 2018 – Oct 25, 2018

Subsystems (2)

Real time clock (RTC) Arm/spreadtrum SoC support

lore

Share & tools

lore

Baolin Wang

author

2018-10-18 08:52 UTC #0 lore raw

cover Fix some issues for RTC alarm function

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

Signature · 1 line

1.7.9.5

Baolin Wang

author

2018-10-18 08:52 UTC #1 lore raw

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

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

Signed-off-by Baolin Wang <[email protected]>

---
 drivers/rtc/rtc-sc27xx.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/rtc/rtc-sc27xx.c +3 -1

index deea5c3..8afba12 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Baolin Wang

author

2018-10-18 08:52 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/rtc/rtc-sc27xx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/rtc/rtc-sc27xx.c +2 -1

index 8afba12..6ac5653 100644
@@ -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,

Signature · 1 line

1.7.9.5

Baolin Wang

author

2018-10-18 08:52 UTC #3 lore raw

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

In reply to 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 <[email protected]>

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

drivers/rtc/rtc-sc27xx.c +0 ~~-20~~

index 6ac5653..4ecabe6 100644
@@ -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) {

Signature · 1 line

1.7.9.5

Baolin Wang

author

2018-10-18 08:52 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/rtc/rtc-sc27xx.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

drivers/rtc/rtc-sc27xx.c +33 -0

index 4ecabe6..72bb002 100644
@@ -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) {

Signature · 1 line

1.7.9.5

Baolin Wang

author

2018-10-18 08:53 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/rtc/rtc-sc27xx.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/rtc/rtc-sc27xx.c +6 -2

index 72bb002..b4eb3b3 100644
@@ -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);

Signature · 1 line

1.7.9.5

Alexandre Belloni

maintainer

2018-10-25 00:34 UTC #6 lore raw

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

Hello,

Quoted 18 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-sc27xx.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/rtc/rtc-sc27xx.c +0 -0
index 72bb002..b4eb3b3 100644
@@ -415,10 +415,14 @@ static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u32 val;
 
 	/*

drivers/rtc/rtc-sc27xx.c static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) · line 425

Quoted 10 lines

drivers/rtc/rtc-sc27xx.c +6 -2

-	 * 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.

Signature · 3 lines

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

Baolin Wang

author

2018-10-25 01:57 UTC #7 lore raw

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

Hi Alexandre,

Quoted 29 lines · Alexandre Belloni

Hello,

Quoted 26 lines · Baolin Wang

Signed-off-by Baolin Wang <[email protected]>

---
 drivers/rtc/rtc-sc27xx.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/rtc/rtc-sc27xx.c +5 -1

index 72bb002..b4eb3b3 100644
@@ -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.
       */

Quoted 8 lines

drivers/rtc/rtc-sc27xx.c static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) · line 425
Quoted 2 lines
drivers/rtc/rtc-sc27xx.c +1 -1
-     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.

Signature · 2 lines

Baolin Wang
Best Regards

Alexandre Belloni

maintainer

2018-10-25 00:25 UTC #8 lore raw

In reply to Baolin Wang

Quoted 9 lines · 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

Quoted 3 lines

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

All applied, thanks.

Signature · 3 lines

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