Message-ID: <201404021233.s32CXeco057975@swsrvapps-02.lan>
Date:	Wed, 2 Apr 2014 12:45:38 +0100
From:	"Opensource [Anthony Olech]" <anthony.olech.opensource@...semi.com>
To:	Alessandro Zummo <a.zummo@...ertech.it>,
	Support Opensource <support.opensource@...semi.com>
CC:	<linux-kernel@...r.kernel.org>, <rtc-linux@...glegroups.com>,
	David Dajun Chen <david.chen@...semi.com>
Subject: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm

Setting the alarm to a time not on a minute boundary results in repeated
interrupts being generated by the DA9052/3 PMIC device until the kernel
RTC core sees that the alarm has rung. Sometimes the number and frequency
of interrupts can cause the kernel to disable the IRQ line used by the
DA9052/3 PMIC with disasterous consequences. This patch fixes the problem.

Even though the DA9052/3 PMIC is capable generating periodic interrupts,
ie TICKS, the method used to distinguish RTC_AF from RTC_PF events was
flawed and can not work in conjunction with the regmap_irq kernel core.
Thus that flawed detection has also been removed by the DA9052/3 PMIC RTC
driver's irq handler, so that it no longer reports the wrong type of event
to the kernel RTC core.

The internal static functions within the DA9052/3 PMIC RTC driver have
been changed to pass the 'da9052_rtc' structure instead of the 'da9052'
because there is no backwards pointer from the 'da9052' structure.

Signed-off-by: Anthony Olech <anthony.olech.opensource@...semi.com>
---

This patch is relative to linux-next repository tag next-20140402

This patch fixes the three issues described above. The first is serious
because usiing the RTC alarm set to a non minute boundary will eventually
cause all component drivers that depend on the interrupt line to fail. The
solution adopted is to round up to alarm time to the next highest minute.

The second bug, reporting a RTC_PF event instead of an RTC_AF event turns
out to not matter with the current implementation of the kernel RTC core
as it seems to ignore the event type. However, should that change in the
future it is better to fix the issue now and not have 'problems waiting to
happen'

The third set of changes are to make the da9052_rtc structure available
to all the local internal functions in the driver. This was done during
testing so that diagnostic data could be stored there. Should the solution
to the first issue be found not acceptable, then the alternative of using
the TICKS interrupt at the fixed one second interval in order to step to
the exact second of the requested alarm requires an extra (alarm time)
piece of data to be stored. In devices that use the alarm function to wake
up from sleep, accuracy to the second will result in the device being
awake for up to nearly a minute longer than expected.

 drivers/rtc/rtc-da9052.c |  122 +++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 56 deletions(-)

diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c
index a1cbf64..e5c9486 100644
--- a/drivers/rtc/rtc-da9052.c
+++ b/drivers/rtc/rtc-da9052.c
@@ -20,28 +20,28 @@
 #include <linux/mfd/da9052/da9052.h>
 #include <linux/mfd/da9052/reg.h>
 
-#define rtc_err(da9052, fmt, ...) \
-		dev_err(da9052->dev, "%s: " fmt, __func__, ##__VA_ARGS__)
+#define rtc_err(rtc, fmt, ...) \
+		dev_err(rtc->da9052->dev, "%s: " fmt, __func__, ##__VA_ARGS__)
 
 struct da9052_rtc {
 	struct rtc_device *rtc;
 	struct da9052 *da9052;
 };
 
-static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
+static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable)
 {
 	int ret;
 	if (enable) {
-		ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
-					DA9052_ALARM_Y_ALARM_ON,
-					DA9052_ALARM_Y_ALARM_ON);
+		ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG,
+				DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON,
+				DA9052_ALARM_Y_ALARM_ON);
 		if (ret != 0)
-			rtc_err(da9052, "Failed to enable ALM: %d\n", ret);
+			rtc_err(rtc, "Failed to enable ALM: %d\n", ret);
 	} else {
-		ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
-					DA9052_ALARM_Y_ALARM_ON, 0);
+		ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG,
+			DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0);
 		if (ret != 0)
-			rtc_err(da9052, "Write error: %d\n", ret);
+			rtc_err(rtc, "Write error: %d\n", ret);
 	}
 	return ret;
 }
@@ -49,31 +49,20 @@ static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
 static irqreturn_t da9052_rtc_irq(int irq, void *data)
 {
 	struct da9052_rtc *rtc = data;
-	int ret;
 
-	ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_MI_REG);
-	if (ret < 0) {
-		rtc_err(rtc->da9052, "Read error: %d\n", ret);
-		return IRQ_NONE;
-	}
-
-	if (ret & DA9052_ALARMMI_ALARMTYPE) {
-		da9052_rtc_enable_alarm(rtc->da9052, 0);
-		rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
-	} else
-		rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_PF);
+	rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
 
 	return IRQ_HANDLED;
 }
 
-static int da9052_read_alarm(struct da9052 *da9052, struct rtc_time *rtc_tm)
+static int da9052_read_alarm(struct da9052_rtc *rtc, struct rtc_time *rtc_tm)
 {
 	int ret;
 	uint8_t v[5];
 
-	ret = da9052_group_read(da9052, DA9052_ALARM_MI_REG, 5, v);
+	ret = da9052_group_read(rtc->da9052, DA9052_ALARM_MI_REG, 5, v);
 	if (ret != 0) {
-		rtc_err(da9052, "Failed to group read ALM: %d\n", ret);
+		rtc_err(rtc, "Failed to group read ALM: %d\n", ret);
 		return ret;
 	}
 
@@ -84,23 +73,33 @@ static int da9052_read_alarm(struct da9052 *da9052, struct rtc_time *rtc_tm)
 	rtc_tm->tm_min  = v[0] & DA9052_RTC_MIN;
 
 	ret = rtc_valid_tm(rtc_tm);
-	if (ret != 0)
-		return ret;
 	return ret;
 }
 
-static int da9052_set_alarm(struct da9052 *da9052, struct rtc_time *rtc_tm)
+static int da9052_set_alarm(struct da9052_rtc *rtc, struct rtc_time *rtc_tm)
 {
+	struct da9052 *da9052 = rtc->da9052;
+	unsigned long alm_time;
 	int ret;
 	uint8_t v[3];
 
+	ret = rtc_tm_to_time(rtc_tm, &alm_time);
+	if (ret != 0)
+		return ret;
+
+	if (rtc_tm->tm_sec > 0) {
+		alm_time += 60 - rtc_tm->tm_sec;
+		rtc_time_to_tm(alm_time, rtc_tm);
+	}
+	BUG_ON(rtc_tm->tm_sec); /* it will cause repeated irqs if not zero */
+
 	rtc_tm->tm_year -= 100;
 	rtc_tm->tm_mon += 1;
 
 	ret = da9052_reg_update(da9052, DA9052_ALARM_MI_REG,
 				DA9052_RTC_MIN, rtc_tm->tm_min);
 	if (ret != 0) {
-		rtc_err(da9052, "Failed to write ALRM MIN: %d\n", ret);
+		rtc_err(rtc, "Failed to write ALRM MIN: %d\n", ret);
 		return ret;
 	}
 
@@ -115,22 +114,22 @@ static int da9052_set_alarm(struct da9052 *da9052, struct rtc_time *rtc_tm)
 	ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
 				DA9052_RTC_YEAR, rtc_tm->tm_year);
 	if (ret != 0)
-		rtc_err(da9052, "Failed to write ALRM YEAR: %d\n", ret);
+		rtc_err(rtc, "Failed to write ALRM YEAR: %d\n", ret);
 
 	return ret;
 }
 
-static int da9052_rtc_get_alarm_status(struct da9052 *da9052)
+static int da9052_rtc_get_alarm_status(struct da9052_rtc *rtc)
 {
 	int ret;
 
-	ret = da9052_reg_read(da9052, DA9052_ALARM_Y_REG);
+	ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_Y_REG);
 	if (ret < 0) {
-		rtc_err(da9052, "Failed to read ALM: %d\n", ret);
+		rtc_err(rtc, "Failed to read ALM: %d\n", ret);
 		return ret;
 	}
-	ret &= DA9052_ALARM_Y_ALARM_ON;
-	return (ret > 0) ? 1 : 0;
+
+	return !!(ret&DA9052_ALARM_Y_ALARM_ON);
 }
 
 static int da9052_rtc_read_time(struct device *dev, struct rtc_time *rtc_tm)
@@ -141,7 +140,7 @@ static int da9052_rtc_read_time(struct device *dev, struct rtc_time *rtc_tm)
 
 	ret = da9052_group_read(rtc->da9052, DA9052_COUNT_S_REG, 6, v);
 	if (ret < 0) {
-		rtc_err(rtc->da9052, "Failed to read RTC time : %d\n", ret);
+		rtc_err(rtc, "Failed to read RTC time : %d\n", ret);
 		return ret;
 	}
 
@@ -153,18 +152,14 @@ static int da9052_rtc_read_time(struct device *dev, struct rtc_time *rtc_tm)
 	rtc_tm->tm_sec  = v[0] & DA9052_RTC_SEC;
 
 	ret = rtc_valid_tm(rtc_tm);
-	if (ret != 0) {
-		rtc_err(rtc->da9052, "rtc_valid_tm failed: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static int da9052_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct da9052_rtc *rtc;
 	uint8_t v[6];
+	int ret;
 
 	rtc = dev_get_drvdata(dev);
 
@@ -175,7 +170,10 @@ static int da9052_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	v[4] = tm->tm_mon + 1;
 	v[5] = tm->tm_year - 100;
 
-	return da9052_group_write(rtc->da9052, DA9052_COUNT_S_REG, 6, v);
+	ret = da9052_group_write(rtc->da9052, DA9052_COUNT_S_REG, 6, v);
+	if (ret < 0)
+		rtc_err(rtc, "failed to set RTC time: %d\n", ret);
+	return ret;
 }
 
 static int da9052_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
@@ -184,13 +182,13 @@ static int da9052_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct rtc_time *tm = &alrm->time;
 	struct da9052_rtc *rtc = dev_get_drvdata(dev);
 
-	ret = da9052_read_alarm(rtc->da9052, tm);
-
-	if (ret)
+	ret = da9052_read_alarm(rtc, tm);
+	if (ret < 0) {
+		rtc_err(rtc, "failed to read RTC alarm: %d\n", ret);
 		return ret;
+	}
 
-	alrm->enabled = da9052_rtc_get_alarm_status(rtc->da9052);
-
+	alrm->enabled = da9052_rtc_get_alarm_status(rtc);
 	return 0;
 }
 
@@ -200,16 +198,15 @@ static int da9052_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct rtc_time *tm = &alrm->time;
 	struct da9052_rtc *rtc = dev_get_drvdata(dev);
 
-	ret = da9052_rtc_enable_alarm(rtc->da9052, 0);
+	ret = da9052_rtc_enable_alarm(rtc, 0);
 	if (ret < 0)
 		return ret;
 
-	ret = da9052_set_alarm(rtc->da9052, tm);
-	if (ret)
+	ret = da9052_set_alarm(rtc, tm);
+	if (ret < 0)
 		return ret;
 
-	ret = da9052_rtc_enable_alarm(rtc->da9052, 1);
-
+	ret = da9052_rtc_enable_alarm(rtc, 1);
 	return ret;
 }
 
@@ -217,7 +214,7 @@ static int da9052_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct da9052_rtc *rtc = dev_get_drvdata(dev);
 
-	return da9052_rtc_enable_alarm(rtc->da9052, enabled);
+	return da9052_rtc_enable_alarm(rtc, enabled);
 }
 
 static const struct rtc_class_ops da9052_rtc_ops = {
@@ -239,10 +236,23 @@ static int da9052_rtc_probe(struct platform_device *pdev)
 
 	rtc->da9052 = dev_get_drvdata(pdev->dev.parent);
 	platform_set_drvdata(pdev, rtc);
+
+	ret = da9052_reg_write(rtc->da9052, DA9052_BBAT_CONT_REG, 0xFE);
+	if (ret < 0) {
+		rtc_err(rtc,
+			"Failed to setup RTC battery charging: %d\n", ret);
+		return ret;
+	}
+
+	ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG,
+				DA9052_ALARM_Y_TICK_ON, 0);
+	if (ret != 0)
+		rtc_err(rtc, "Failed to disable TICKS: %d\n", ret);
+
 	ret = da9052_request_irq(rtc->da9052, DA9052_IRQ_ALARM, "ALM",
 				da9052_rtc_irq, rtc);
 	if (ret != 0) {
-		rtc_err(rtc->da9052, "irq registration failed: %d\n", ret);
+		rtc_err(rtc, "irq registration failed: %d\n", ret);
 		return ret;
 	}
 
@@ -261,7 +271,7 @@ static struct platform_driver da9052_rtc_driver = {
 
 module_platform_driver(da9052_rtc_driver);
 
-MODULE_AUTHOR("David Dajun Chen <dchen@...semi.com>");
+MODULE_AUTHOR("Anthony Olech <Anthony.Olech@...semi.com>");
 MODULE_DESCRIPTION("RTC driver for Dialog DA9052 PMIC");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:da9052-rtc");
-- 
end-of-patch 1/1 for drivers/rtc: da9052: ALARM causes interrupt storm V1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists