[v2] rtc: s3c: Rewrite clock handling

Message ID 20190121110930.8290-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • [v2] rtc: s3c: Rewrite clock handling
Related show

Commit Message

Marek Szyprowski Jan. 21, 2019, 11:09 a.m.
s3c_rtc_enable/disable_clk() functions were designed to be called multiple
times without reference counting, because they were initially only used in
alarm setting/clearing functions, which can be called both when alarm is
already set or not. Later however, calls to those functions have been added to
other places in the driver - like time and /proc reading callbacks, what
results in broken alarm if any of such events happens after the alarm has
been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions
to rely on proper reference counting in clock core and move alarm enable
counter to s3c_rtc_setaie() function.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
v2:
- simplified code as suggested by Krzysztof
---
 drivers/rtc/rtc-s3c.c | 74 ++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 43 deletions(-)

-- 
2.17.1

Comments

Krzysztof Kozlowski Jan. 21, 2019, 3:33 p.m. | #1
On Mon, 21 Jan 2019 at 12:09, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> s3c_rtc_enable/disable_clk() functions were designed to be called multiple

> times without reference counting, because they were initially only used in

> alarm setting/clearing functions, which can be called both when alarm is

> already set or not. Later however, calls to those functions have been added to

> other places in the driver - like time and /proc reading callbacks, what

> results in broken alarm if any of such events happens after the alarm has

> been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions

> to rely on proper reference counting in clock core and move alarm enable

> counter to s3c_rtc_setaie() function.

>

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

> v2:

> - simplified code as suggested by Krzysztof

> ---

>  drivers/rtc/rtc-s3c.c | 74 ++++++++++++++++++-------------------------

>  1 file changed, 31 insertions(+), 43 deletions(-)


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof
Alexandre Belloni Jan. 22, 2019, 5:35 p.m. | #2
On 21/01/2019 12:09:30+0100, Marek Szyprowski wrote:
> s3c_rtc_enable/disable_clk() functions were designed to be called multiple

> times without reference counting, because they were initially only used in

> alarm setting/clearing functions, which can be called both when alarm is

> already set or not. Later however, calls to those functions have been added to

> other places in the driver - like time and /proc reading callbacks, what

> results in broken alarm if any of such events happens after the alarm has

> been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions

> to rely on proper reference counting in clock core and move alarm enable

> counter to s3c_rtc_setaie() function.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

> v2:

> - simplified code as suggested by Krzysztof

> ---

>  drivers/rtc/rtc-s3c.c | 74 ++++++++++++++++++-------------------------

>  1 file changed, 31 insertions(+), 43 deletions(-)

> 

Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Patch

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 04c68178c42d..ea82efba6f94 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -39,7 +39,7 @@  struct s3c_rtc {
 	void __iomem *base;
 	struct clk *rtc_clk;
 	struct clk *rtc_src_clk;
-	bool clk_disabled;
+	bool alarm_enabled;
 
 	const struct s3c_rtc_data *data;
 
@@ -47,7 +47,7 @@  struct s3c_rtc {
 	int irq_tick;
 
 	spinlock_t pie_lock;
-	spinlock_t alarm_clk_lock;
+	spinlock_t alarm_lock;
 
 	int ticnt_save;
 	int ticnt_en_save;
@@ -70,44 +70,27 @@  struct s3c_rtc_data {
 
 static int s3c_rtc_enable_clk(struct s3c_rtc *info)
 {
-	unsigned long irq_flags;
-	int ret = 0;
+	int ret;
 
-	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
+	ret = clk_enable(info->rtc_clk);
+	if (ret)
+		return ret;
 
-	if (info->clk_disabled) {
-		ret = clk_enable(info->rtc_clk);
-		if (ret)
-			goto out;
-
-		if (info->data->needs_src_clk) {
-			ret = clk_enable(info->rtc_src_clk);
-			if (ret) {
-				clk_disable(info->rtc_clk);
-				goto out;
-			}
+	if (info->data->needs_src_clk) {
+		ret = clk_enable(info->rtc_src_clk);
+		if (ret) {
+			clk_disable(info->rtc_clk);
+			return ret;
 		}
-		info->clk_disabled = false;
 	}
-
-out:
-	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
-
-	return ret;
+	return 0;
 }
 
 static void s3c_rtc_disable_clk(struct s3c_rtc *info)
 {
-	unsigned long irq_flags;
-
-	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
-	if (!info->clk_disabled) {
-		if (info->data->needs_src_clk)
-			clk_disable(info->rtc_src_clk);
-		clk_disable(info->rtc_clk);
-		info->clk_disabled = true;
-	}
-	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
+	clk_disable(info->rtc_clk);
 }
 
 /* IRQ Handlers */
@@ -135,6 +118,7 @@  static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
+	unsigned long flags;
 	unsigned int tmp;
 	int ret;
 
@@ -151,17 +135,19 @@  static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 
 	writeb(tmp, info->base + S3C2410_RTCALM);
 
-	s3c_rtc_disable_clk(info);
+	spin_lock_irqsave(&info->alarm_lock, flags);
 
-	if (enabled) {
-		ret = s3c_rtc_enable_clk(info);
-		if (ret)
-			return ret;
-	} else {
+	if (info->alarm_enabled && !enabled)
 		s3c_rtc_disable_clk(info);
-	}
+	else if (!info->alarm_enabled && enabled)
+		ret = s3c_rtc_enable_clk(info);
 
-	return 0;
+	info->alarm_enabled = enabled;
+	spin_unlock_irqrestore(&info->alarm_lock, flags);
+
+	s3c_rtc_disable_clk(info);
+
+	return ret;
 }
 
 /* Set RTC frequency */
@@ -357,10 +343,10 @@  static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	writeb(alrm_en, info->base + S3C2410_RTCALM);
 
-	s3c_rtc_disable_clk(info);
-
 	s3c_rtc_setaie(dev, alrm->enabled);
 
+	s3c_rtc_disable_clk(info);
+
 	return 0;
 }
 
@@ -491,7 +477,7 @@  static int s3c_rtc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 	spin_lock_init(&info->pie_lock);
-	spin_lock_init(&info->alarm_clk_lock);
+	spin_lock_init(&info->alarm_lock);
 
 	platform_set_drvdata(pdev, info);
 
@@ -591,6 +577,8 @@  static int s3c_rtc_probe(struct platform_device *pdev)
 
 	s3c_rtc_setfreq(info, 1);
 
+	s3c_rtc_disable_clk(info);
+
 	return 0;
 
 err_nortc: