From: Thomas Gleixner <tglx@linutronix.de> To: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexander Dahl <ada@thorsis.com>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Date: Wed, 20 Jun 2018 12:07:00 +0200 (CEST) [thread overview] Message-ID: <alpine.DEB.2.21.1806201156510.10546@nanos.tec.linutronix.de> (raw) In-Reply-To: <20180620094649.GA2766@piout.net> On Wed, 20 Jun 2018, Alexandre Belloni wrote: > On 20/06/2018 11:03:40+0200, Thomas Gleixner wrote: > > > +/* > > > + * Clocksource and clockevent using the same channel(s) > > > + */ > > > +static u64 tc_get_cycles(struct clocksource *cs) > > > +{ > > > + u32 lower, upper; > > > + > > > + do { > > > + upper = readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[1])); > > > + lower = readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + } while (upper != readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[1]))); > > > + > > > + return (upper << 16) | lower; > > > +} > > > > For timekeeping the win of this is dubious. With a 5Mhz clock the 32bit > > part wraps around in ~859 seconds, which is plenty even for NOHZ. > > > > So I really would avoid the double read/compare/eventually repeat magic and > > just use the lower 32bits for performance sake. I assume the same is true > > for sched_clock(), but I might be wrong. > > > > Agreed, this is why this is only used with the 16 bit counters (the > register is 32 bit wide but the counter only have 16 bits. For the 32 > bit counters, tc_get_cycles32 is used and only use one counter. Ah, sorry. I misread the code. Missed that it's the 16bit case. > > > +static int tcb_clkevt_next_event(unsigned long delta, > > > + struct clock_event_device *d) > > > +{ > > > + u32 old, next, cur; > > > + > > > + old = readl(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + next = old + delta; > > > + writel(next, tc.base + ATMEL_TC_RC(tc.channels[0])); > > > + cur = readl(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + > > > + /* check whether the delta elapsed while setting the register */ > > > + if ((next < old && cur < old && cur > next) || > > > + (next > old && (cur < old || cur > next))) { > > > + /* > > > + * Clear the CPCS bit in the status register to avoid > > > + * generating a spurious interrupt next time a valid > > > + * timer event is configured. > > > + */ > > > + old = readl(tc.base + ATMEL_TC_SR(tc.channels[0])); > > > + return -ETIME; > > > + } > > > > Aarg. Doesn;t that timer block have a simple count down and fire mode? > > These compare equal timers suck. > > It only counts up... Have you tried to play with that waveform stuff? Thanks, tglx
WARNING: multiple messages have this Message-ID (diff)
From: tglx@linutronix.de (Thomas Gleixner) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Date: Wed, 20 Jun 2018 12:07:00 +0200 (CEST) [thread overview] Message-ID: <alpine.DEB.2.21.1806201156510.10546@nanos.tec.linutronix.de> (raw) In-Reply-To: <20180620094649.GA2766@piout.net> On Wed, 20 Jun 2018, Alexandre Belloni wrote: > On 20/06/2018 11:03:40+0200, Thomas Gleixner wrote: > > > +/* > > > + * Clocksource and clockevent using the same channel(s) > > > + */ > > > +static u64 tc_get_cycles(struct clocksource *cs) > > > +{ > > > + u32 lower, upper; > > > + > > > + do { > > > + upper = readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[1])); > > > + lower = readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + } while (upper != readl_relaxed(tc.base + ATMEL_TC_CV(tc.channels[1]))); > > > + > > > + return (upper << 16) | lower; > > > +} > > > > For timekeeping the win of this is dubious. With a 5Mhz clock the 32bit > > part wraps around in ~859 seconds, which is plenty even for NOHZ. > > > > So I really would avoid the double read/compare/eventually repeat magic and > > just use the lower 32bits for performance sake. I assume the same is true > > for sched_clock(), but I might be wrong. > > > > Agreed, this is why this is only used with the 16 bit counters (the > register is 32 bit wide but the counter only have 16 bits. For the 32 > bit counters, tc_get_cycles32 is used and only use one counter. Ah, sorry. I misread the code. Missed that it's the 16bit case. > > > +static int tcb_clkevt_next_event(unsigned long delta, > > > + struct clock_event_device *d) > > > +{ > > > + u32 old, next, cur; > > > + > > > + old = readl(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + next = old + delta; > > > + writel(next, tc.base + ATMEL_TC_RC(tc.channels[0])); > > > + cur = readl(tc.base + ATMEL_TC_CV(tc.channels[0])); > > > + > > > + /* check whether the delta elapsed while setting the register */ > > > + if ((next < old && cur < old && cur > next) || > > > + (next > old && (cur < old || cur > next))) { > > > + /* > > > + * Clear the CPCS bit in the status register to avoid > > > + * generating a spurious interrupt next time a valid > > > + * timer event is configured. > > > + */ > > > + old = readl(tc.base + ATMEL_TC_SR(tc.channels[0])); > > > + return -ETIME; > > > + } > > > > Aarg. Doesn;t that timer block have a simple count down and fire mode? > > These compare equal timers suck. > > It only counts up... Have you tried to play with that waveform stuff? Thanks, tglx
next prev parent reply other threads:[~2018-06-20 10:07 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-19 21:19 [PATCH v5 0/6] clocksource: rework Atmel TCB timer driver Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-19 21:19 ` [PATCH v5 1/6] ARM: at91: add TCB registers definitions Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-28 15:15 ` Daniel Lezcano 2018-06-28 15:15 ` Daniel Lezcano 2018-06-28 18:34 ` Alexandre Belloni 2018-06-28 18:34 ` Alexandre Belloni 2018-06-28 19:55 ` Daniel Lezcano 2018-06-28 19:55 ` Daniel Lezcano 2018-06-19 21:19 ` [PATCH v5 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-20 9:03 ` Thomas Gleixner 2018-06-20 9:03 ` Thomas Gleixner 2018-06-20 9:46 ` Alexandre Belloni 2018-06-20 9:46 ` Alexandre Belloni 2018-06-20 10:07 ` Thomas Gleixner [this message] 2018-06-20 10:07 ` Thomas Gleixner 2018-06-20 10:32 ` Alexandre Belloni 2018-06-20 10:32 ` Alexandre Belloni 2018-06-20 10:58 ` Thomas Gleixner 2018-06-20 10:58 ` Thomas Gleixner 2018-06-20 11:18 ` Alexandre Belloni 2018-06-20 11:18 ` Alexandre Belloni 2018-06-20 11:55 ` Thomas Gleixner 2018-06-20 11:55 ` Thomas Gleixner 2018-06-28 16:24 ` Daniel Lezcano 2018-06-28 16:24 ` Daniel Lezcano 2018-07-06 15:18 ` Alexandre Belloni 2018-07-06 15:18 ` Alexandre Belloni 2018-06-19 21:19 ` [PATCH v5 3/6] clocksource/drivers: atmel-pit: make option silent Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-19 21:19 ` [PATCH v5 4/6] ARM: at91: Implement clocksource selection Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-19 21:19 ` [PATCH v5 5/6] ARM: configs: at91: use new TCB timer driver Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni 2018-06-19 21:19 ` [PATCH v5 6/6] ARM: configs: at91: unselect PIT Alexandre Belloni 2018-06-19 21:19 ` Alexandre Belloni
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=alpine.DEB.2.21.1806201156510.10546@nanos.tec.linutronix.de \ --to=tglx@linutronix.de \ --cc=ada@thorsis.com \ --cc=alexandre.belloni@bootlin.com \ --cc=bigeasy@linutronix.de \ --cc=daniel.lezcano@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nicolas.ferre@microchip.com \ /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.