From: Tomasz Figa <tomasz.figa@gmail.com> To: linux-arm-kernel@lists.infradead.org Cc: Thomas Gleixner <tglx@linutronix.de>, Maxime Ripard <maxime.ripard@free-electrons.com>, Emilio Lopez <emilio@elopez.com.ar>, linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com, John Stultz <john.stultz@linaro.org>, sunny@allwinnertech.com, shuge@allwinnertech.com, kevin@allwinnertech.com Subject: Re: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code Date: Fri, 28 Jun 2013 22:35:29 +0200 [thread overview] Message-ID: <7300725.devmrYGi4t@flatron> (raw) In-Reply-To: <alpine.DEB.2.02.1306282204220.4013@ionos.tec.linutronix.de> On Friday 28 of June 2013 22:13:08 Thomas Gleixner wrote: > On Fri, 28 Jun 2013, Maxime Ripard wrote: > > The next_event logic was setting the next interval to fire in the > > current timer value instead of the interval value register, which is > > obviously wrong. > > Ok. > > > Plus the logic to set the actual value was wrong as well, so this > > code has always been broken. > > This lacks an explanation why the logic is wrong and what the actual > fix is. > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > > > drivers/clocksource/sun4i_timer.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/clocksource/sun4i_timer.c > > b/drivers/clocksource/sun4i_timer.c index 84ace76..695c8c8 100644 > > --- a/drivers/clocksource/sun4i_timer.c > > +++ b/drivers/clocksource/sun4i_timer.c > > @@ -16,6 +16,7 @@ > > > > #include <linux/clk.h> > > #include <linux/clockchips.h> > > > > +#include <linux/delay.h> > > > > #include <linux/interrupt.h> > > #include <linux/irq.h> > > #include <linux/irqreturn.h> > > > > @@ -61,9 +62,14 @@ static void sun4i_clkevt_mode(enum clock_event_mode > > mode,> > > static int sun4i_clkevt_next_event(unsigned long evt, > > > > struct clock_event_device *unused) > > > > { > > > > - u32 u = readl(timer_base + TIMER_CTL_REG(0)); > > - writel(evt, timer_base + TIMER_CNTVAL_REG(0)); > > - writel(u | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD, > > + u32 val = readl(timer_base + TIMER_CTL_REG(0)); > > + writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0)); > > + udelay(1); > > That udelay() is more than suspicious. Not only it is suspicious, but also delays the event by 1 microsecond. Not much, given usual usage of clock events, but still. >From what I understand from this code, you keep this timer running and just stop it to set new event. Can you simply disable autoreload and just program this timer to start counting from evt down to 0 when it generates interrupt and just stops itself? I believe this would simplify the logic a bit, but is it possible with this hardware? Best regards, Tomasz
WARNING: multiple messages have this Message-ID (diff)
From: tomasz.figa@gmail.com (Tomasz Figa) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code Date: Fri, 28 Jun 2013 22:35:29 +0200 [thread overview] Message-ID: <7300725.devmrYGi4t@flatron> (raw) In-Reply-To: <alpine.DEB.2.02.1306282204220.4013@ionos.tec.linutronix.de> On Friday 28 of June 2013 22:13:08 Thomas Gleixner wrote: > On Fri, 28 Jun 2013, Maxime Ripard wrote: > > The next_event logic was setting the next interval to fire in the > > current timer value instead of the interval value register, which is > > obviously wrong. > > Ok. > > > Plus the logic to set the actual value was wrong as well, so this > > code has always been broken. > > This lacks an explanation why the logic is wrong and what the actual > fix is. > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > > > drivers/clocksource/sun4i_timer.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/clocksource/sun4i_timer.c > > b/drivers/clocksource/sun4i_timer.c index 84ace76..695c8c8 100644 > > --- a/drivers/clocksource/sun4i_timer.c > > +++ b/drivers/clocksource/sun4i_timer.c > > @@ -16,6 +16,7 @@ > > > > #include <linux/clk.h> > > #include <linux/clockchips.h> > > > > +#include <linux/delay.h> > > > > #include <linux/interrupt.h> > > #include <linux/irq.h> > > #include <linux/irqreturn.h> > > > > @@ -61,9 +62,14 @@ static void sun4i_clkevt_mode(enum clock_event_mode > > mode,> > > static int sun4i_clkevt_next_event(unsigned long evt, > > > > struct clock_event_device *unused) > > > > { > > > > - u32 u = readl(timer_base + TIMER_CTL_REG(0)); > > - writel(evt, timer_base + TIMER_CNTVAL_REG(0)); > > - writel(u | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD, > > + u32 val = readl(timer_base + TIMER_CTL_REG(0)); > > + writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0)); > > + udelay(1); > > That udelay() is more than suspicious. Not only it is suspicious, but also delays the event by 1 microsecond. Not much, given usual usage of clock events, but still. >From what I understand from this code, you keep this timer running and just stop it to set new event. Can you simply disable autoreload and just program this timer to start counting from evt down to 0 when it generates interrupt and just stops itself? I believe this would simplify the logic a bit, but is it possible with this hardware? Best regards, Tomasz
next prev parent reply other threads:[~2013-06-28 20:35 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-06-28 19:56 [PATCHv2 0/8] clocksource: sunxi: Timer fixes and cleanup Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 1/8] clocksource: sun4i: Use the BIT macros where possible Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 2/8] clocksource: sun4i: Add clocksource and sched clock drivers Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 3/8] clocksource: sun4i: Don't forget to enable the clock we use Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 4/8] clocksource: sun4i: Fix the next event code Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 20:13 ` Thomas Gleixner 2013-06-28 20:13 ` Thomas Gleixner 2013-06-28 20:35 ` Tomasz Figa [this message] 2013-06-28 20:35 ` Tomasz Figa 2013-06-28 21:20 ` Maxime Ripard 2013-06-28 21:20 ` Maxime Ripard 2013-06-28 21:08 ` Maxime Ripard 2013-06-28 21:08 ` Maxime Ripard 2013-06-28 21:27 ` Thomas Gleixner 2013-06-28 21:27 ` Thomas Gleixner 2013-06-29 6:48 ` Maxime Ripard 2013-06-29 6:48 ` Maxime Ripard 2013-07-01 8:15 ` Thomas Gleixner 2013-07-01 8:15 ` Thomas Gleixner 2013-06-28 19:56 ` [PATCHv2 5/8] clocksource: sun4i: Factor out some timer code Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 6/8] clocksource: sun4i: Remove TIMER_SCAL variable Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 7/8] clocksource: sun4i: Cleanup parent clock setup Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard 2013-06-28 19:56 ` [PATCHv2 8/8] clocksource: sun4i: Fix bug when switching from periodic to oneshot modes Maxime Ripard 2013-06-28 19:56 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7300725.devmrYGi4t@flatron \ --to=tomasz.figa@gmail.com \ --cc=emilio@elopez.com.ar \ --cc=john.stultz@linaro.org \ --cc=kevin@allwinnertech.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=maxime.ripard@free-electrons.com \ --cc=shuge@allwinnertech.com \ --cc=sunny@allwinnertech.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.