rtc: s3c: Rewrite clock handling

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

Commit Message

Marek Szyprowski Jan. 18, 2019, 1:27 p.m.
s3c_rtc_enable/disable_clk() functions were designed to be called multiple
times without reference counting, because they were initially 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>

---
 drivers/rtc/rtc-s3c.c | 67 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 39 deletions(-)

-- 
2.17.1

Comments

Krzysztof Kozlowski Jan. 19, 2019, 8:17 p.m. | #1
On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:
> s3c_rtc_enable/disable_clk() functions were designed to be called multiple

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


s/used/used only/
(if I understand correctly the logic)

> 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>

> ---

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

>  1 file changed, 28 insertions(+), 39 deletions(-)

> 

> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c

> index 04c68178c42d..e682977b4f6e 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;


Maybe add short comment that it protects only "alarm_enabled" property?

>  

>  	int ticnt_save;

>  	int ticnt_en_save;

> @@ -70,44 +70,28 @@ struct s3c_rtc_data {

>  

>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)

>  {

> -	unsigned long irq_flags;

>  	int ret = 0;

>  

> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);

> +	ret = clk_enable(info->rtc_clk);

> +	if (ret)

> +		goto out;


The out label is now empty so just "return ret". It is easier to read -
no need to jump anywhere to see the simple return.

>  

> -	if (info->clk_disabled) {

> -		ret = clk_enable(info->rtc_clk);

> -		if (ret)

> +	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);

> -				goto out;

> -			}

>  		}

> -		info->clk_disabled = false;

>  	}

> -

>  out:

> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);

> -

>  	return ret;

>  }

>  

>  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 +119,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 +136,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 +344,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);


I do not understand this change - why do you have to move the disable
clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for
the time of accessing registers.

> +

>  	return 0;

>  }

>  

> @@ -491,7 +478,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 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)

>  

>  	s3c_rtc_setfreq(info, 1);

>  

> +	s3c_rtc_disable_clk(info);


I cannot find the reason why this is related to this particular change.
I mean, it looks reasonable because previously the clock looked like it
was enabled all the time... but maybe this should be separate pach?


Best regards,
Krzysztof

> +

>  	return 0;

>  

>  err_nortc:

> -- 

> 2.17.1

>
Marek Szyprowski Jan. 21, 2019, 8:39 a.m. | #2
Hi Krzysztof,

On 2019-01-19 21:17, Krzysztof Kozlowski wrote:
> On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:

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

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

> s/used/used only/

> (if I understand correctly the logic)

>

>> 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>

>> ---

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

>>  1 file changed, 28 insertions(+), 39 deletions(-)

>>

>> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c

>> index 04c68178c42d..e682977b4f6e 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;

> Maybe add short comment that it protects only "alarm_enabled" property?

>

>>  

>>  	int ticnt_save;

>>  	int ticnt_en_save;

>> @@ -70,44 +70,28 @@ struct s3c_rtc_data {

>>  

>>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)

>>  {

>> -	unsigned long irq_flags;

>>  	int ret = 0;

>>  

>> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);

>> +	ret = clk_enable(info->rtc_clk);

>> +	if (ret)

>> +		goto out;

> The out label is now empty so just "return ret". It is easier to read -

> no need to jump anywhere to see the simple return.

>

>>  

>> -	if (info->clk_disabled) {

>> -		ret = clk_enable(info->rtc_clk);

>> -		if (ret)

>> +	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);

>> -				goto out;

>> -			}

>>  		}

>> -		info->clk_disabled = false;

>>  	}

>> -

>>  out:

>> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);

>> -

>>  	return ret;

>>  }

>>  

>>  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 +119,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 +136,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 +344,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);

> I do not understand this change - why do you have to move the disable

> clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for

> the time of accessing registers.


This was micro optimization, s3c_rtc_setaie() increases clock enable
count, so by changing the call order we can avoid one disable/enable
sequence.

>> +

>>  	return 0;

>>  }

>>  

>> @@ -491,7 +478,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 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)

>>  

>>  	s3c_rtc_setfreq(info, 1);

>>  

>> +	s3c_rtc_disable_clk(info);

> I cannot find the reason why this is related to this particular change.

> I mean, it looks reasonable because previously the clock looked like it

> was enabled all the time... but maybe this should be separate pach?


It wasn't enabled all the time, because the call to s3c_rtc_setfreq()
disabled it (remember there was no clock enable reference counting!).
Now once we have enable/disable reference counting, we need to keep them
balanced.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Krzysztof Kozlowski Jan. 21, 2019, 9:59 a.m. | #3
On Mon, 21 Jan 2019 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> Hi Krzysztof,

>

> On 2019-01-19 21:17, Krzysztof Kozlowski wrote:

> > On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:

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

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

> > s/used/used only/

> > (if I understand correctly the logic)

> >

> >> 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>

> >> ---

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

> >>  1 file changed, 28 insertions(+), 39 deletions(-)

> >>

> >> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c

> >> index 04c68178c42d..e682977b4f6e 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;

> > Maybe add short comment that it protects only "alarm_enabled" property?

> >

> >>

> >>      int ticnt_save;

> >>      int ticnt_en_save;

> >> @@ -70,44 +70,28 @@ struct s3c_rtc_data {

> >>

> >>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)

> >>  {

> >> -    unsigned long irq_flags;

> >>      int ret = 0;

> >>

> >> -    spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);

> >> +    ret = clk_enable(info->rtc_clk);

> >> +    if (ret)

> >> +            goto out;

> > The out label is now empty so just "return ret". It is easier to read -

> > no need to jump anywhere to see the simple return.

> >

> >>

> >> -    if (info->clk_disabled) {

> >> -            ret = clk_enable(info->rtc_clk);

> >> -            if (ret)

> >> +    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);

> >> -                            goto out;

> >> -                    }

> >>              }

> >> -            info->clk_disabled = false;

> >>      }

> >> -

> >>  out:

> >> -    spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);

> >> -

> >>      return ret;

> >>  }

> >>

> >>  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 +119,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 +136,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 +344,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);

> > I do not understand this change - why do you have to move the disable

> > clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for

> > the time of accessing registers.

>

> This was micro optimization, s3c_rtc_setaie() increases clock enable

> count, so by changing the call order we can avoid one disable/enable

> sequence.


OK.

> >> +

> >>      return 0;

> >>  }

> >>

> >> @@ -491,7 +478,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 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)

> >>

> >>      s3c_rtc_setfreq(info, 1);

> >>

> >> +    s3c_rtc_disable_clk(info);

> > I cannot find the reason why this is related to this particular change.

> > I mean, it looks reasonable because previously the clock looked like it

> > was enabled all the time... but maybe this should be separate pach?

>

> It wasn't enabled all the time, because the call to s3c_rtc_setfreq()

> disabled it (remember there was no clock enable reference counting!).

> Now once we have enable/disable reference counting, we need to keep them

> balanced.


I get it, thanks!

Best regards,
Krzysztof

Patch

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 04c68178c42d..e682977b4f6e 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,28 @@  struct s3c_rtc_data {
 
 static int s3c_rtc_enable_clk(struct s3c_rtc *info)
 {
-	unsigned long irq_flags;
 	int ret = 0;
 
-	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
+	ret = clk_enable(info->rtc_clk);
+	if (ret)
+		goto out;
 
-	if (info->clk_disabled) {
-		ret = clk_enable(info->rtc_clk);
-		if (ret)
+	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);
-				goto out;
-			}
 		}
-		info->clk_disabled = false;
 	}
-
 out:
-	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
-
 	return ret;
 }
 
 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 +119,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 +136,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 +344,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 +478,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 +578,8 @@  static int s3c_rtc_probe(struct platform_device *pdev)
 
 	s3c_rtc_setfreq(info, 1);
 
+	s3c_rtc_disable_clk(info);
+
 	return 0;
 
 err_nortc: