RE: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm

From: Opensource [Anthony Olech]
Date: Wed Apr 30 2014 - 04:21:00 EST

Next message: Tony Prisk: "Re: [PATCH] pinctrl: vt8500: Ensure value reg is updated when setting direction"
Previous message: Hans de Goede: "Re: [PATCH v2] mba6x_bl: Backlight driver for mid 2013 MacBook Air"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Alessandro,
you did not reply to my patch submission.
Why not?
Tony Olech
Dialog Semiconductor

> -----Original Message-----
> From: Opensource [Anthony Olech]
> [mailto:anthony.olech.opensource@xxxxxxxxxxx]
> Sent: 02 April 2014 12:46
> To: Alessandro Zummo; Support Opensource
> Cc: linux-kernel@xxxxxxxxxxxxxxx; rtc-linux@xxxxxxxxxxxxxxxx; David Dajun
> Chen
> 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@xxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxx>");
> +MODULE_AUTHOR("Anthony Olech <Anthony.Olech@xxxxxxxxxxx>");
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tony Prisk: "Re: [PATCH] pinctrl: vt8500: Ensure value reg is updated when setting direction"
Previous message: Hans de Goede: "Re: [PATCH v2] mba6x_bl: Backlight driver for mid 2013 MacBook Air"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]