| Message ID | 20190118132754.15660-1-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: s3c: Rewrite clock handling | expand |
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 >
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
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
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:
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