Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932290AbaD3IU4 (ORCPT ); Wed, 30 Apr 2014 04:20:56 -0400 Received: from mail1.bemta5.messagelabs.com ([195.245.231.152]:58466 "EHLO mail1.bemta5.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757415AbaD3IUx convert rfc822-to-8bit (ORCPT ); Wed, 30 Apr 2014 04:20:53 -0400 X-Env-Sender: anthony.olech.opensource@diasemi.com X-Msg-Ref: server-10.tower-178.messagelabs.com!1398846048!25950091!1 X-Originating-IP: [82.210.246.133] X-StarScan-Received: X-StarScan-Version: 6.11.3; banners=-,-,- X-VirusChecked: Checked From: "Opensource [Anthony Olech]" To: Alessandro Zummo , "Greg Kroah-Hartman (gregkh@linuxfoundation.org)" CC: "linux-kernel@vger.kernel.org" , "rtc-linux@googlegroups.com" , "Opensource [Anthony Olech]" , Support Opensource Subject: RE: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm Thread-Topic: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm Thread-Index: AQHPTm/SRU3jGWxxrU63CAe4ChnpwZsp/JfA Date: Wed, 30 Apr 2014 08:20:45 +0000 Message-ID: <24DF37198A1E704D9811D8F72B87EB51D98D9437@NB-EX-MBX02.diasemi.com> References: <201404021233.s32CXeco057975@swsrvapps-02.lan> In-Reply-To: <201404021233.s32CXeco057975@swsrvapps-02.lan> Accept-Language: en-GB, de-DE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.20.24.126] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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@diasemi.com] > Sent: 02 April 2014 12:46 > To: Alessandro Zummo; Support Opensource > Cc: linux-kernel@vger.kernel.org; rtc-linux@googlegroups.com; 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 > --- > > 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 > #include > > -#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 "); > +MODULE_AUTHOR("Anthony Olech "); > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/