From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752917Ab3F1VUY (ORCPT ); Fri, 28 Jun 2013 17:20:24 -0400 Received: from mail.free-electrons.com ([94.23.35.102]:34105 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752020Ab3F1VUW (ORCPT ); Fri, 28 Jun 2013 17:20:22 -0400 Date: Fri, 28 Jun 2013 23:20:19 +0200 From: Maxime Ripard To: Tomasz Figa Cc: linux-arm-kernel@lists.infradead.org, Thomas Gleixner , Emilio Lopez , linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com, John Stultz , sunny@allwinnertech.com, shuge@allwinnertech.com, kevin@allwinnertech.com Subject: Re: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code Message-ID: <20130628212019.GC2756@lukather> References: <1372449386-1334-1-git-send-email-maxime.ripard@free-electrons.com> <1372449386-1334-5-git-send-email-maxime.ripard@free-electrons.com> <7300725.devmrYGi4t@flatron> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="JWEK1jqKZ6MHAcjA" Content-Disposition: inline In-Reply-To: <7300725.devmrYGi4t@flatron> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --JWEK1jqKZ6MHAcjA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jun 28, 2013 at 10:35:29PM +0200, Tomasz Figa wrote: > 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. > >=20 > > Ok. > >=20 > > > Plus the logic to set the actual value was wrong as well, so this > > > code has always been broken. > >=20 > > This lacks an explanation why the logic is wrong and what the actual > > fix is. > >=20 > > > Signed-off-by: Maxime Ripard > > > --- > > >=20 > > > drivers/clocksource/sun4i_timer.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > >=20 > > > 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 @@ > > >=20 > > > #include > > > #include > > >=20 > > > +#include > > >=20 > > > #include > > > #include > > > #include > > >=20 > > > @@ -61,9 +62,14 @@ static void sun4i_clkevt_mode(enum clock_event_mode > > > mode,>=20 > > > static int sun4i_clkevt_next_event(unsigned long evt, > > > =20 > > > struct clock_event_device *unused) > > > =20 > > > { > > >=20 > > > - u32 u =3D 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 =3D readl(timer_base + TIMER_CTL_REG(0)); > > > + writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0)); > > > + udelay(1); > >=20 > > That udelay() is more than suspicious. >=20 > Not only it is suspicious, but also delays the event by 1 microsecond. No= t=20 > much, given usual usage of clock events, but still. >=20 > From what I understand from this code, you keep this timer running and=20 > just stop it to set new event. Can you simply disable autoreload and just= =20 > program this timer to start counting from evt down to 0 when it generates= =20 > interrupt and just stops itself? Something like that, but not completely, because the timer actually stops. To reprogram a new interval to a running timer, you have to: - Disable it - Program the new interval - Propagates the new interval and start the timer by setting the bits ENABLE and (AUTO)RELOAD (AUTORELOAD is probably a bad name here actually). That is, wether or not it's a oneshot or periodic timer. Now, between the time you disable the timer and enable it back, you have to wait at least 2 timer clock source cycles (which is around 85ns). It's the ONESHOT (BIT(7)) that actually controls wether or not the timer is periodic. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --JWEK1jqKZ6MHAcjA Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJRzf4TAAoJEBx+YmzsjxAgEXsP/ipgR0ILSkO5eGbSlwJvbDpX IvXHJGHPbx9Tg/y+XDRfTNHWTUoSc7TJHjxBnUhNDKDr8D4DVe1mY1rtttpRGy5x Bfqf4UdnCdzQfpMgvHoFjMwKaXdPu1eUBgxVoATuSaoiTHOHAomMMbyB/u5BIg/3 kD64NJMcguYUXPN9FzBDBEv6R3NT0K2B3VOmrjaiC+4fPMJVNfa9n4K5s35APX6p yv94QPhAvgVcjhIJ8OMFweQtFDNIJNj4xlydruCa/qZGALXEF9Cw3E12fPl1i9Mx EiJ4kGHHLx+ULBYcsbuhmMeYmqXumM73jUKkEWKERl84wpy03lYJ5r1CursJgbJ/ nXkZqZLub61fmGipvAxD2dYVjCDEcpqOGUKGhDnpE57G5twhnlALTy8cYwTheLXe 8Atf7Zf36+F8sUx1ul29TrjRCI6gMCaIaWtjfx6yGCVfT9J9lwXEgJIzZ+kpSdfB aqaVtQ/pbwEIpU50uh0EUifZij39BEScJRWKdBKvB4KnF+7GvbDIqdEU1DaPjsIx thd9EbU4qrBeimymPScuOMTOwKkn1Ks7DvqQ7mfit26x2LXCio943AmFo3otHssq DGCObFI1dFGDcXtSbGVglHLVuI7+f3pKiozSoxqnXJv7JU2UZaEzHFgeY300dqRp 4YjFzs4v+o6y5E5Q/mx5 =16Xw -----END PGP SIGNATURE----- --JWEK1jqKZ6MHAcjA-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Fri, 28 Jun 2013 23:20:19 +0200 Subject: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code In-Reply-To: <7300725.devmrYGi4t@flatron> References: <1372449386-1334-1-git-send-email-maxime.ripard@free-electrons.com> <1372449386-1334-5-git-send-email-maxime.ripard@free-electrons.com> <7300725.devmrYGi4t@flatron> Message-ID: <20130628212019.GC2756@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Jun 28, 2013 at 10:35:29PM +0200, Tomasz Figa wrote: > 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 > > > --- > > > > > > 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 > > > #include > > > > > > +#include > > > > > > #include > > > #include > > > #include > > > > > > @@ -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? Something like that, but not completely, because the timer actually stops. To reprogram a new interval to a running timer, you have to: - Disable it - Program the new interval - Propagates the new interval and start the timer by setting the bits ENABLE and (AUTO)RELOAD (AUTORELOAD is probably a bad name here actually). That is, wether or not it's a oneshot or periodic timer. Now, between the time you disable the timer and enable it back, you have to wait at least 2 timer clock source cycles (which is around 85ns). It's the ONESHOT (BIT(7)) that actually controls wether or not the timer is periodic. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: