[PATCH v2 0/3] rtc: omap: Fix misc bugs

[PATCH v2 0/3] rtc: omap: Fix misc bugs

[Lokesh Vutla]

This patch series fixes miscellaneous bugs in OMAP RTC driver.

Tested on BeagleBone-Black.
Logs: http://pastebin.ubuntu.com/10723287/

V1 patches can be seen here: http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg115839.html

Lokesh Vutla (3):
  rtc: omap: Unlock and Lock rtc registers before and after register
    writes
  rtc: omap: Update Kconfig for OMAP RTC
  rtc: omap: use module_platform_driver

 drivers/rtc/Kconfig    |  6 ++---
 drivers/rtc/rtc-omap.c | 68 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 58 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] rtc: omap: Unlock and Lock rtc registers before and after register writes

[Lokesh Vutla]

RTC module contains a kicker mechanism to prevent any spurious writes
from changing the register values. This mechanism requires two MMR
writes to the KICK0 and KICK1 registers with exact data values
before the kicker lock mechanism is released.

Currently the driver release the lock in the probe and leaves it enabled
until the rtc driver removal. This eliminates the idea of preventing
spurious writes when RTC driver is loaded.
So implement rtc lock and unlock functions before and after register writes.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
-- This is as advised by Paul to implement lock and unlock functions in
  the driver and not to unlock and leave it in probe.
  The same discussion can be seen here:
  http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg111588.html
- Changes since v1:
  -Instead of testing for has_kicker each time, added .lock and
   .unlock to omap_rtc_device_type.

 drivers/rtc/rtc-omap.c | 63 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 8e5851a..935212c 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -118,12 +118,15 @@
 #define	KICK0_VALUE			0x83e70b13
 #define	KICK1_VALUE			0x95a4f1e0
 
+struct omap_rtc;
+
 struct omap_rtc_device_type {
 	bool has_32kclk_en;
-	bool has_kicker;
 	bool has_irqwakeen;
 	bool has_pmic_mode;
 	bool has_power_up_reset;
+	void (*lock)(struct omap_rtc *rtc);
+	void (*unlock)(struct omap_rtc *rtc);
 };
 
 struct omap_rtc {
@@ -156,6 +159,26 @@ static inline void rtc_writel(struct omap_rtc *rtc, unsigned int reg, u32 val)
 	writel(val, rtc->base + reg);
 }
 
+static inline void am3352_rtc_unlock(struct omap_rtc *rtc)
+{
+	rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
+	rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
+}
+
+static inline void am3352_rtc_lock(struct omap_rtc *rtc)
+{
+	rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc_writel(rtc, OMAP_RTC_KICK1_REG, 0);
+}
+
+static inline void default_rtc_unlock(struct omap_rtc *rtc)
+{
+}
+
+static inline void default_rtc_lock(struct omap_rtc *rtc)
+{
+}
+
 /*
  * We rely on the rtc framework to handle locking (rtc->ops_lock),
  * so the only other requirement is that register accesses which
@@ -186,7 +209,9 @@ static irqreturn_t rtc_irq(int irq, void *dev_id)
 
 	/* alarm irq? */
 	if (irq_data & OMAP_RTC_STATUS_ALARM) {
+		rtc->type->unlock(rtc);
 		rtc_write(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM);
+		rtc->type->lock(rtc);
 		events |= RTC_IRQF | RTC_AF;
 	}
 
@@ -218,9 +243,11 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 		irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN;
 	}
 	rtc_wait_not_busy(rtc);
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
 	if (rtc->type->has_irqwakeen)
 		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
+	rtc->type->lock(rtc);
 	local_irq_enable();
 
 	return 0;
@@ -293,12 +320,14 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	local_irq_disable();
 	rtc_wait_not_busy(rtc);
 
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_YEARS_REG, tm->tm_year);
 	rtc_write(rtc, OMAP_RTC_MONTHS_REG, tm->tm_mon);
 	rtc_write(rtc, OMAP_RTC_DAYS_REG, tm->tm_mday);
 	rtc_write(rtc, OMAP_RTC_HOURS_REG, tm->tm_hour);
 	rtc_write(rtc, OMAP_RTC_MINUTES_REG, tm->tm_min);
 	rtc_write(rtc, OMAP_RTC_SECONDS_REG, tm->tm_sec);
+	rtc->type->lock(rtc);
 
 	local_irq_enable();
 
@@ -341,6 +370,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	local_irq_disable();
 	rtc_wait_not_busy(rtc);
 
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_ALARM_YEARS_REG, alm->time.tm_year);
 	rtc_write(rtc, OMAP_RTC_ALARM_MONTHS_REG, alm->time.tm_mon);
 	rtc_write(rtc, OMAP_RTC_ALARM_DAYS_REG, alm->time.tm_mday);
@@ -362,6 +392,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
 	if (rtc->type->has_irqwakeen)
 		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
+	rtc->type->lock(rtc);
 
 	local_irq_enable();
 
@@ -391,6 +422,7 @@ static void omap_rtc_power_off(void)
 	unsigned long now;
 	u32 val;
 
+	rtc->type->unlock(rtc);
 	/* enable pmic_power_en control */
 	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
 	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
@@ -423,6 +455,7 @@ static void omap_rtc_power_off(void)
 	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
 			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
+	rtc->type->lock(rtc);
 
 	/*
 	 * Wait for alarm to trigger (within two seconds) and external PMIC to
@@ -442,17 +475,21 @@ static struct rtc_class_ops omap_rtc_ops = {
 
 static const struct omap_rtc_device_type omap_rtc_default_type = {
 	.has_power_up_reset = true,
+	.lock		= default_rtc_lock,
+	.unlock		= default_rtc_unlock,
 };
 
 static const struct omap_rtc_device_type omap_rtc_am3352_type = {
 	.has_32kclk_en	= true,
-	.has_kicker	= true,
 	.has_irqwakeen	= true,
 	.has_pmic_mode	= true,
+	.lock		= am3352_rtc_lock,
+	.unlock		= am3352_rtc_unlock,
 };
 
 static const struct omap_rtc_device_type omap_rtc_da830_type = {
-	.has_kicker	= true,
+	.lock		= am3352_rtc_lock,
+	.unlock		= am3352_rtc_unlock,
 };
 
 static const struct platform_device_id omap_rtc_id_table[] = {
@@ -527,10 +564,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	if (rtc->type->has_kicker) {
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
-		rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
-	}
+	rtc->type->unlock(rtc);
 
 	/*
 	 * disable interrupts
@@ -593,6 +627,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 	if (reg != new_ctrl)
 		rtc_write(rtc, OMAP_RTC_CTRL_REG, new_ctrl);
 
+	rtc->type->lock(rtc);
+
 	device_init_wakeup(&pdev->dev, true);
 
 	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
@@ -626,8 +662,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 
 err:
 	device_init_wakeup(&pdev->dev, false);
-	if (rtc->type->has_kicker)
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc->type->lock(rtc);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -646,11 +681,11 @@ static int __exit omap_rtc_remove(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 0);
 
+	rtc->type->unlock(rtc);
 	/* leave rtc running, but disable irqs */
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
 
-	if (rtc->type->has_kicker)
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc->type->lock(rtc);
 
 	/* Disable the clock/module */
 	pm_runtime_put_sync(&pdev->dev);
@@ -666,6 +701,7 @@ static int omap_rtc_suspend(struct device *dev)
 
 	rtc->interrupts_reg = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 
+	rtc->type->unlock(rtc);
 	/*
 	 * FIXME: the RTC alarm is not currently acting as a wakeup event
 	 * source on some platforms, and in fact this enable() call is just
@@ -675,6 +711,7 @@ static int omap_rtc_suspend(struct device *dev)
 		enable_irq_wake(rtc->irq_alarm);
 	else
 		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
+	rtc->type->lock(rtc);
 
 	/* Disable the clock/module */
 	pm_runtime_put_sync(dev);
@@ -689,10 +726,12 @@ static int omap_rtc_resume(struct device *dev)
 	/* Enable the clock/module so that we can access the registers */
 	pm_runtime_get_sync(dev);
 
+	rtc->type->unlock(rtc);
 	if (device_may_wakeup(dev))
 		disable_irq_wake(rtc->irq_alarm);
 	else
 		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, rtc->interrupts_reg);
+	rtc->type->lock(rtc);
 
 	return 0;
 }
@@ -709,9 +748,11 @@ static void omap_rtc_shutdown(struct platform_device *pdev)
 	 * Keep the ALARM interrupt enabled to allow the system to power up on
 	 * alarm events.
 	 */
+	rtc->type->unlock(rtc);
 	mask = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 	mask &= OMAP_RTC_INTERRUPTS_IT_ALARM;
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, mask);
+	rtc->type->lock(rtc);
 }
 
 static struct platform_driver omap_rtc_driver = {
-- 
1.9.1

Re: [PATCH v2 1/3] rtc: omap: Unlock and Lock rtc registers before and after register writes

[Alexandre Belloni]

On 02/04/2015 at 16:39:09 +0530, Lokesh Vutla wrote :
> RTC module contains a kicker mechanism to prevent any spurious writes
> from changing the register values. This mechanism requires two MMR
> writes to the KICK0 and KICK1 registers with exact data values
> before the kicker lock mechanism is released.
> 
> Currently the driver release the lock in the probe and leaves it enabled
> until the rtc driver removal. This eliminates the idea of preventing
> spurious writes when RTC driver is loaded.
> So implement rtc lock and unlock functions before and after register writes.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> -- This is as advised by Paul to implement lock and unlock functions in
>   the driver and not to unlock and leave it in probe.
>   The same discussion can be seen here:
>   http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg111588.html
> - Changes since v1:
>   -Instead of testing for has_kicker each time, added .lock and
>    .unlock to omap_rtc_device_type.
> 
>  drivers/rtc/rtc-omap.c | 63 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index 8e5851a..935212c 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -118,12 +118,15 @@
>  #define	KICK0_VALUE			0x83e70b13
>  #define	KICK1_VALUE			0x95a4f1e0
>  
> +struct omap_rtc;
> +
>  struct omap_rtc_device_type {
>  	bool has_32kclk_en;
> -	bool has_kicker;
>  	bool has_irqwakeen;
>  	bool has_pmic_mode;
>  	bool has_power_up_reset;
> +	void (*lock)(struct omap_rtc *rtc);
> +	void (*unlock)(struct omap_rtc *rtc);
>  };
>  
>  struct omap_rtc {
> @@ -156,6 +159,26 @@ static inline void rtc_writel(struct omap_rtc *rtc, unsigned int reg, u32 val)
>  	writel(val, rtc->base + reg);
>  }
>  
> +static inline void am3352_rtc_unlock(struct omap_rtc *rtc)
> +{
> +	rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
> +	rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
> +}
> +
> +static inline void am3352_rtc_lock(struct omap_rtc *rtc)
> +{
> +	rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
> +	rtc_writel(rtc, OMAP_RTC_KICK1_REG, 0);
> +}
> +
> +static inline void default_rtc_unlock(struct omap_rtc *rtc)
> +{
> +}
> +
> +static inline void default_rtc_lock(struct omap_rtc *rtc)
> +{
> +}
> +

As they are called through a pointer, it is unnecessary to declare the
functions as inlined, they will not be inlined anyway.

Else, you can add my ack.

>  /*
>   * We rely on the rtc framework to handle locking (rtc->ops_lock),
>   * so the only other requirement is that register accesses which
> @@ -186,7 +209,9 @@ static irqreturn_t rtc_irq(int irq, void *dev_id)
>  
>  	/* alarm irq? */
>  	if (irq_data & OMAP_RTC_STATUS_ALARM) {
> +		rtc->type->unlock(rtc);
>  		rtc_write(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM);
> +		rtc->type->lock(rtc);
>  		events |= RTC_IRQF | RTC_AF;
>  	}
>  
> @@ -218,9 +243,11 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  		irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN;
>  	}
>  	rtc_wait_not_busy(rtc);
> +	rtc->type->unlock(rtc);
>  	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
>  	if (rtc->type->has_irqwakeen)
>  		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
> +	rtc->type->lock(rtc);
>  	local_irq_enable();
>  
>  	return 0;
> @@ -293,12 +320,14 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	local_irq_disable();
>  	rtc_wait_not_busy(rtc);
>  
> +	rtc->type->unlock(rtc);
>  	rtc_write(rtc, OMAP_RTC_YEARS_REG, tm->tm_year);
>  	rtc_write(rtc, OMAP_RTC_MONTHS_REG, tm->tm_mon);
>  	rtc_write(rtc, OMAP_RTC_DAYS_REG, tm->tm_mday);
>  	rtc_write(rtc, OMAP_RTC_HOURS_REG, tm->tm_hour);
>  	rtc_write(rtc, OMAP_RTC_MINUTES_REG, tm->tm_min);
>  	rtc_write(rtc, OMAP_RTC_SECONDS_REG, tm->tm_sec);
> +	rtc->type->lock(rtc);
>  
>  	local_irq_enable();
>  
> @@ -341,6 +370,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	local_irq_disable();
>  	rtc_wait_not_busy(rtc);
>  
> +	rtc->type->unlock(rtc);
>  	rtc_write(rtc, OMAP_RTC_ALARM_YEARS_REG, alm->time.tm_year);
>  	rtc_write(rtc, OMAP_RTC_ALARM_MONTHS_REG, alm->time.tm_mon);
>  	rtc_write(rtc, OMAP_RTC_ALARM_DAYS_REG, alm->time.tm_mday);
> @@ -362,6 +392,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
>  	if (rtc->type->has_irqwakeen)
>  		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
> +	rtc->type->lock(rtc);
>  
>  	local_irq_enable();
>  
> @@ -391,6 +422,7 @@ static void omap_rtc_power_off(void)
>  	unsigned long now;
>  	u32 val;
>  
> +	rtc->type->unlock(rtc);
>  	/* enable pmic_power_en control */
>  	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
>  	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
> @@ -423,6 +455,7 @@ static void omap_rtc_power_off(void)
>  	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>  	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
>  			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
> +	rtc->type->lock(rtc);
>  
>  	/*
>  	 * Wait for alarm to trigger (within two seconds) and external PMIC to
> @@ -442,17 +475,21 @@ static struct rtc_class_ops omap_rtc_ops = {
>  
>  static const struct omap_rtc_device_type omap_rtc_default_type = {
>  	.has_power_up_reset = true,
> +	.lock		= default_rtc_lock,
> +	.unlock		= default_rtc_unlock,
>  };
>  
>  static const struct omap_rtc_device_type omap_rtc_am3352_type = {
>  	.has_32kclk_en	= true,
> -	.has_kicker	= true,
>  	.has_irqwakeen	= true,
>  	.has_pmic_mode	= true,
> +	.lock		= am3352_rtc_lock,
> +	.unlock		= am3352_rtc_unlock,
>  };
>  
>  static const struct omap_rtc_device_type omap_rtc_da830_type = {
> -	.has_kicker	= true,
> +	.lock		= am3352_rtc_lock,
> +	.unlock		= am3352_rtc_unlock,
>  };
>  
>  static const struct platform_device_id omap_rtc_id_table[] = {
> @@ -527,10 +564,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  
> -	if (rtc->type->has_kicker) {
> -		rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
> -		rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
> -	}
> +	rtc->type->unlock(rtc);
>  
>  	/*
>  	 * disable interrupts
> @@ -593,6 +627,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>  	if (reg != new_ctrl)
>  		rtc_write(rtc, OMAP_RTC_CTRL_REG, new_ctrl);
>  
> +	rtc->type->lock(rtc);
> +
>  	device_init_wakeup(&pdev->dev, true);
>  
>  	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> @@ -626,8 +662,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>  
>  err:
>  	device_init_wakeup(&pdev->dev, false);
> -	if (rtc->type->has_kicker)
> -		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
> +	rtc->type->lock(rtc);
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> @@ -646,11 +681,11 @@ static int __exit omap_rtc_remove(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, 0);
>  
> +	rtc->type->unlock(rtc);
>  	/* leave rtc running, but disable irqs */
>  	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
>  
> -	if (rtc->type->has_kicker)
> -		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
> +	rtc->type->lock(rtc);
>  
>  	/* Disable the clock/module */
>  	pm_runtime_put_sync(&pdev->dev);
> @@ -666,6 +701,7 @@ static int omap_rtc_suspend(struct device *dev)
>  
>  	rtc->interrupts_reg = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>  
> +	rtc->type->unlock(rtc);
>  	/*
>  	 * FIXME: the RTC alarm is not currently acting as a wakeup event
>  	 * source on some platforms, and in fact this enable() call is just
> @@ -675,6 +711,7 @@ static int omap_rtc_suspend(struct device *dev)
>  		enable_irq_wake(rtc->irq_alarm);
>  	else
>  		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
> +	rtc->type->lock(rtc);
>  
>  	/* Disable the clock/module */
>  	pm_runtime_put_sync(dev);
> @@ -689,10 +726,12 @@ static int omap_rtc_resume(struct device *dev)
>  	/* Enable the clock/module so that we can access the registers */
>  	pm_runtime_get_sync(dev);
>  
> +	rtc->type->unlock(rtc);
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(rtc->irq_alarm);
>  	else
>  		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, rtc->interrupts_reg);
> +	rtc->type->lock(rtc);
>  
>  	return 0;
>  }
> @@ -709,9 +748,11 @@ static void omap_rtc_shutdown(struct platform_device *pdev)
>  	 * Keep the ALARM interrupt enabled to allow the system to power up on
>  	 * alarm events.
>  	 */
> +	rtc->type->unlock(rtc);
>  	mask = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
>  	mask &= OMAP_RTC_INTERRUPTS_IT_ALARM;
>  	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, mask);
> +	rtc->type->lock(rtc);
>  }
>  
>  static struct platform_driver omap_rtc_driver = {
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 1/3] rtc: omap: Unlock and Lock rtc registers before and after register writes

[Lokesh Vutla]

On Thursday 02 April 2015 06:03 PM, Alexandre Belloni wrote:
> On 02/04/2015 at 16:39:09 +0530, Lokesh Vutla wrote :

[..snip..]
>>  }
>>  
>> +static inline void am3352_rtc_unlock(struct omap_rtc *rtc)
>> +{
>> +	rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
>> +	rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
>> +}
>> +
>> +static inline void am3352_rtc_lock(struct omap_rtc *rtc)
>> +{
>> +	rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
>> +	rtc_writel(rtc, OMAP_RTC_KICK1_REG, 0);
>> +}
>> +
>> +static inline void default_rtc_unlock(struct omap_rtc *rtc)
>> +{
>> +}
>> +
>> +static inline void default_rtc_lock(struct omap_rtc *rtc)
>> +{
>> +}
>> +
> 
> As they are called through a pointer, it is unnecessary to declare the
> functions as inlined, they will not be inlined anyway.

Thats true, thanks for pointing it.
Here is the updated patch.


-------8<--------------------------------
>From 04c5241b56f751bb1ffd70ecbd526bbbe26d6251 Mon Sep 17 00:00:00 2001
From: Lokesh Vutla <lokeshvutla@ti.com>
Date: Wed, 1 Apr 2015 10:10:56 +0530
Subject: [PATCH v3 1/3] rtc: omap: Unlock and Lock rtc registers before and
 after register writes

RTC module contains a kicker mechanism to prevent any spurious writes
from changing the register values. This mechanism requires two MMR
writes to the KICK0 and KICK1 registers with exact data values
before the kicker lock mechanism is released.

Currently the driver release the lock in the probe and leaves it enabled
until the rtc driver removal. This eliminates the idea of preventing
spurious writes when RTC driver is loaded.
So implement rtc lock and unlock functions before and after register writes.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
-- This is as advised by Paul to implement lock and unlock functions in
  the driver and not to unlock and leave it in probe.
  The same discussion can be seen here:
  http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg111588.html
- Changes since v1:
  -Instead of testing for has_kicker each time, added .lock and
   .unlock to omap_rtc_device_type.
- Changes since v2:
  -Removed inline for lock and unlock functions.

 drivers/rtc/rtc-omap.c | 63 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 8e5851a..935212c 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -118,12 +118,15 @@
 #define	KICK0_VALUE			0x83e70b13
 #define	KICK1_VALUE			0x95a4f1e0
 
+struct omap_rtc;
+
 struct omap_rtc_device_type {
 	bool has_32kclk_en;
-	bool has_kicker;
 	bool has_irqwakeen;
 	bool has_pmic_mode;
 	bool has_power_up_reset;
+	void (*lock)(struct omap_rtc *rtc);
+	void (*unlock)(struct omap_rtc *rtc);
 };
 
 struct omap_rtc {
@@ -156,6 +159,26 @@ static inline void rtc_writel(struct omap_rtc *rtc, unsigned int reg, u32 val)
 	writel(val, rtc->base + reg);
 }
 
+static void am3352_rtc_unlock(struct omap_rtc *rtc)
+{
+	rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
+	rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
+}
+
+static void am3352_rtc_lock(struct omap_rtc *rtc)
+{
+	rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc_writel(rtc, OMAP_RTC_KICK1_REG, 0);
+}
+
+static void default_rtc_unlock(struct omap_rtc *rtc)
+{
+}
+
+static void default_rtc_lock(struct omap_rtc *rtc)
+{
+}
+
 /*
  * We rely on the rtc framework to handle locking (rtc->ops_lock),
  * so the only other requirement is that register accesses which
@@ -186,7 +209,9 @@ static irqreturn_t rtc_irq(int irq, void *dev_id)
 
 	/* alarm irq? */
 	if (irq_data & OMAP_RTC_STATUS_ALARM) {
+		rtc->type->unlock(rtc);
 		rtc_write(rtc, OMAP_RTC_STATUS_REG, OMAP_RTC_STATUS_ALARM);
+		rtc->type->lock(rtc);
 		events |= RTC_IRQF | RTC_AF;
 	}
 
@@ -218,9 +243,11 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 		irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN;
 	}
 	rtc_wait_not_busy(rtc);
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
 	if (rtc->type->has_irqwakeen)
 		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
+	rtc->type->lock(rtc);
 	local_irq_enable();
 
 	return 0;
@@ -293,12 +320,14 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	local_irq_disable();
 	rtc_wait_not_busy(rtc);
 
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_YEARS_REG, tm->tm_year);
 	rtc_write(rtc, OMAP_RTC_MONTHS_REG, tm->tm_mon);
 	rtc_write(rtc, OMAP_RTC_DAYS_REG, tm->tm_mday);
 	rtc_write(rtc, OMAP_RTC_HOURS_REG, tm->tm_hour);
 	rtc_write(rtc, OMAP_RTC_MINUTES_REG, tm->tm_min);
 	rtc_write(rtc, OMAP_RTC_SECONDS_REG, tm->tm_sec);
+	rtc->type->lock(rtc);
 
 	local_irq_enable();
 
@@ -341,6 +370,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	local_irq_disable();
 	rtc_wait_not_busy(rtc);
 
+	rtc->type->unlock(rtc);
 	rtc_write(rtc, OMAP_RTC_ALARM_YEARS_REG, alm->time.tm_year);
 	rtc_write(rtc, OMAP_RTC_ALARM_MONTHS_REG, alm->time.tm_mon);
 	rtc_write(rtc, OMAP_RTC_ALARM_DAYS_REG, alm->time.tm_mday);
@@ -362,6 +392,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, reg);
 	if (rtc->type->has_irqwakeen)
 		rtc_write(rtc, OMAP_RTC_IRQWAKEEN, irqwake_reg);
+	rtc->type->lock(rtc);
 
 	local_irq_enable();
 
@@ -391,6 +422,7 @@ static void omap_rtc_power_off(void)
 	unsigned long now;
 	u32 val;
 
+	rtc->type->unlock(rtc);
 	/* enable pmic_power_en control */
 	val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
 	rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
@@ -423,6 +455,7 @@ static void omap_rtc_power_off(void)
 	val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 	rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
 			val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
+	rtc->type->lock(rtc);
 
 	/*
 	 * Wait for alarm to trigger (within two seconds) and external PMIC to
@@ -442,17 +475,21 @@ static struct rtc_class_ops omap_rtc_ops = {
 
 static const struct omap_rtc_device_type omap_rtc_default_type = {
 	.has_power_up_reset = true,
+	.lock		= default_rtc_lock,
+	.unlock		= default_rtc_unlock,
 };
 
 static const struct omap_rtc_device_type omap_rtc_am3352_type = {
 	.has_32kclk_en	= true,
-	.has_kicker	= true,
 	.has_irqwakeen	= true,
 	.has_pmic_mode	= true,
+	.lock		= am3352_rtc_lock,
+	.unlock		= am3352_rtc_unlock,
 };
 
 static const struct omap_rtc_device_type omap_rtc_da830_type = {
-	.has_kicker	= true,
+	.lock		= am3352_rtc_lock,
+	.unlock		= am3352_rtc_unlock,
 };
 
 static const struct platform_device_id omap_rtc_id_table[] = {
@@ -527,10 +564,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	if (rtc->type->has_kicker) {
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, KICK0_VALUE);
-		rtc_writel(rtc, OMAP_RTC_KICK1_REG, KICK1_VALUE);
-	}
+	rtc->type->unlock(rtc);
 
 	/*
 	 * disable interrupts
@@ -593,6 +627,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 	if (reg != new_ctrl)
 		rtc_write(rtc, OMAP_RTC_CTRL_REG, new_ctrl);
 
+	rtc->type->lock(rtc);
+
 	device_init_wakeup(&pdev->dev, true);
 
 	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
@@ -626,8 +662,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
 
 err:
 	device_init_wakeup(&pdev->dev, false);
-	if (rtc->type->has_kicker)
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc->type->lock(rtc);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -646,11 +681,11 @@ static int __exit omap_rtc_remove(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 0);
 
+	rtc->type->unlock(rtc);
 	/* leave rtc running, but disable irqs */
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
 
-	if (rtc->type->has_kicker)
-		rtc_writel(rtc, OMAP_RTC_KICK0_REG, 0);
+	rtc->type->lock(rtc);
 
 	/* Disable the clock/module */
 	pm_runtime_put_sync(&pdev->dev);
@@ -666,6 +701,7 @@ static int omap_rtc_suspend(struct device *dev)
 
 	rtc->interrupts_reg = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 
+	rtc->type->unlock(rtc);
 	/*
 	 * FIXME: the RTC alarm is not currently acting as a wakeup event
 	 * source on some platforms, and in fact this enable() call is just
@@ -675,6 +711,7 @@ static int omap_rtc_suspend(struct device *dev)
 		enable_irq_wake(rtc->irq_alarm);
 	else
 		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
+	rtc->type->lock(rtc);
 
 	/* Disable the clock/module */
 	pm_runtime_put_sync(dev);
@@ -689,10 +726,12 @@ static int omap_rtc_resume(struct device *dev)
 	/* Enable the clock/module so that we can access the registers */
 	pm_runtime_get_sync(dev);
 
+	rtc->type->unlock(rtc);
 	if (device_may_wakeup(dev))
 		disable_irq_wake(rtc->irq_alarm);
 	else
 		rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, rtc->interrupts_reg);
+	rtc->type->lock(rtc);
 
 	return 0;
 }
@@ -709,9 +748,11 @@ static void omap_rtc_shutdown(struct platform_device *pdev)
 	 * Keep the ALARM interrupt enabled to allow the system to power up on
 	 * alarm events.
 	 */
+	rtc->type->unlock(rtc);
 	mask = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 	mask &= OMAP_RTC_INTERRUPTS_IT_ALARM;
 	rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, mask);
+	rtc->type->lock(rtc);
 }
 
 static struct platform_driver omap_rtc_driver = {
-- 
1.9.1


Thanks and regards,
Lokesh

Re: [PATCH v2 1/3] rtc: omap: Unlock and Lock rtc registers before and after register writes

[Alexandre Belloni]

Hi,

On 02/04/2015 at 18:14:11 +0530, Lokesh Vutla wrote :
> Here is the updated patch.
> 

You should probably send it properly using git send-email, have a look
at the --in-reply-to parameter, else, I'm not sure Andrew will be able
to pick it up.

Thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 2/3] rtc: omap: Update Kconfig for OMAP RTC

[Lokesh Vutla]

RTC is present in AM43xx and DRA7xx also. Updating the Kconfig
to depend on ARCH_OMAP or ARCH_DAVINCI

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/rtc/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b5b5c3d..40faf56 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1121,11 +1121,11 @@ config RTC_DRV_IMXDI
 	   will be called "rtc-imxdi".
 
 config RTC_DRV_OMAP
-	tristate "TI OMAP1"
-	depends on ARCH_OMAP15XX || ARCH_OMAP16XX || ARCH_OMAP730 || ARCH_DAVINCI_DA8XX || SOC_AM33XX
+	tristate "TI OMAP Real Time Clock"
+	depends on ARCH_OMAP || ARCH_DAVINCI
 	help
 	  Say "yes" here to support the on chip real time clock
-	  present on TI OMAP1, AM33xx and DA8xx/OMAP-L13x.
+	  present on TI OMAP1, AM33xx, DA8xx/OMAP-L13x, AM43xx and DRA7xx.
 
 	  This driver can also be built as a module, if so, module
 	  will be called rtc-omap.
-- 
1.9.1

[PATCH v2 3/3] rtc: omap: use module_platform_driver

[Lokesh Vutla]

module_platform_driver_probe() prevents driver from requesting probe deferral.
So using module_platform_drive() to support probe deferral.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/rtc/rtc-omap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 935212c..48f2257 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -521,7 +521,7 @@ static const struct of_device_id omap_rtc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, omap_rtc_of_match);
 
-static int __init omap_rtc_probe(struct platform_device *pdev)
+static int omap_rtc_probe(struct platform_device *pdev)
 {
 	struct omap_rtc	*rtc;
 	struct resource	*res;
@@ -756,6 +756,7 @@ static void omap_rtc_shutdown(struct platform_device *pdev)
 }
 
 static struct platform_driver omap_rtc_driver = {
+	.probe		= omap_rtc_probe,
 	.remove		= __exit_p(omap_rtc_remove),
 	.shutdown	= omap_rtc_shutdown,
 	.driver		= {
@@ -766,7 +767,7 @@ static struct platform_driver omap_rtc_driver = {
 	.id_table	= omap_rtc_id_table,
 };
 
-module_platform_driver_probe(omap_rtc_driver, omap_rtc_probe);
+module_platform_driver(omap_rtc_driver);
 
 MODULE_ALIAS("platform:omap_rtc");
 MODULE_AUTHOR("George G. Davis (and others)");
-- 
1.9.1