Message-ID: <24DF37198A1E704D9811D8F72B87EB51D98D9437@NB-EX-MBX02.diasemi.com>
Date:	Wed, 30 Apr 2014 08:20:45 +0000
From:	"Opensource [Anthony Olech]" <anthony.olech.opensource@...semi.com>
To:	Alessandro Zummo <a.zummo@...ertech.it>,
	"Greg Kroah-Hartman (gregkh@...uxfoundation.org)" 
	<gregkh@...uxfoundation.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
	"Opensource [Anthony Olech]" <anthony.olech.opensource@...semi.com>,
	Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm

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@...semi.com]
> Sent: 02 April 2014 12:46
> To: Alessandro Zummo; Support Opensource
> Cc: linux-kernel@...r.kernel.org; rtc-linux@...glegroups.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 <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