From: srinivas.kandagatla@st•com (Srinivas KANDAGATLA)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC 2/8] ARM:global_timer: Add ARM global timer support.
Date: Tue, 14 May 2013 09:46:38 +0100 [thread overview]
Message-ID: <5191F9EE.6070008@st.com> (raw)
In-Reply-To: <CACRpkdZCP=w=4Q3bnuMQxm=Oe-uZke+Cc5NgddM5vsQbgr9E-Q@mail.gmail.com>
Thankyou for the comments.
On 13/05/13 20:05, Linus Walleij wrote:
> On Wed, May 8, 2013 at 4:11 PM, Srinivas KANDAGATLA
> <srinivas.kandagatla@st•com> wrote:
>
> Thomas Gleixner and John Stultz should always be included on timer code review.
>
> This is one reason I really like to move the clocksource/clockevent/etc code out
> of arch/arm, so that people can get their To: line right from MAINTAINERS.
> (Whether it goes to drivers/clocksource or not is another issue.)
>
>> diff --git a/arch/arm/include/asm/global_timer.h b/arch/arm/include/asm/global_timer.h
>
> Using CLKSRC_OF_INIT() this header goes away entirely I guess.
>
>> diff --git a/arch/arm/kernel/global_timer.c b/arch/arm/kernel/global_timer.c
>
>> +#define GT_COUNTER0 0x00
>> +#define GT_COUNTER1 0x04
>
> So two counters, nice.
>
>> +union gt_counter {
>> + cycle_t cycles;
>> + struct {
>> + uint32_t lower;
>> + uint32_t upper;
>
> Just u32 is fine.
It makes sense to remove this struct totally and use u64 as suggested.
>
>> + };
>> +};
>> +
>> +static union gt_counter gt_counter_read(void)
>> +{
>> + union gt_counter res;
>> + uint32_t upper;
>
> u32
>
>> +
>> + upper = readl(gt_base + GT_COUNTER1);
>> + do {
>> + res.upper = upper;
>> + res.lower = readl(gt_base + GT_COUNTER0);
>> + upper = readl(gt_base + GT_COUNTER1);
>> + } while (upper != res.upper);
>> +
>> + return res;
>> +}
>
> I guess this is some cleverness to avoid wrap-around...
> A comment stating what's going on wouldn't hurt.
This cleverness is defined in the datasheet
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407i/CIHGECHJ.html
I will add a comment here.
>
> But why on earth do you need this complicated union
> to hold the result? Have two local u32 variables and return
> a u64 from this function.
>
> Since this will be performance critical, isn't readl_relaxed() suffient
> here, or what's the rationale for just using readl()?
Yes, it makes sense to go for readl_relaxed here as it is device-memory
type.
>
>> +static void gt_compare_set(unsigned long delta, int periodic)
>> +{
>> + union gt_counter counter = gt_counter_read();
>
> So here you get something easy to read like
>
> u64 counter = gt_counter_read();
>
>> + unsigned long ctrl = readl(gt_base + GT_CONTROL);
>> +
>> + BUG_ON(!(ctrl & GT_CONTROL_TIMER_ENABLE));
>> + BUG_ON(ctrl & (GT_CONTROL_COMP_ENABLE |
>> + GT_CONTROL_IRQ_ENABLE |
>> + GT_CONTROL_AUTO_INC));
>
> Do you really have to check this *whenever the counter is set*?
>
> Will it not suffice to do this once during initialization?
>
> It looks like some leftover debug code. It also looks kind of clock
> dangerous, can you explain exactly why this check is here?
I will get this debug out of here.
>
>> +
>> + counter.cycles += delta;
>> + writel(counter.lower, gt_base + GT_COMP0);
>> + writel(counter.upper, gt_base + GT_COMP1);
>
> This is another instance of the union struct making things
> complicated. With a u64 just:
>
> counter += delta;
> writel(lower_32_bits(counter), gt_base + GT_COMP0);
> writel(upper_32_bits(counter), gt_base + GT_COMP1);
>
> As you can see <linux/kernel.h> has really nice helpers in place
> to deal with 64 bit arithmetics.
Same as first comment.
>
>> +
>> + ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE;
>> +
>> + if (periodic) {
>> + writel(delta, gt_base + GT_AUTO_INC);
>> + ctrl |= GT_CONTROL_AUTO_INC;
>> + }
>> +
>> + writel(ctrl, gt_base + GT_CONTROL);
>> +}
>> +
>> +static void gt_clockevent_set_mode(enum clock_event_mode mode,
>> + struct clock_event_device *clk)
>> +{
>> + switch (mode) {
>> + case CLOCK_EVT_MODE_PERIODIC:
>> + gt_compare_set(gt_clk_rate/HZ, 1);
>
> Use
> gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1);
> to get integer arithmetics right.
Will move to use DIV_ROUND_CLOSEST.
>
>> + break;
>> + case CLOCK_EVT_MODE_ONESHOT:
>> + /* period set, and timer enabled in 'next_event' hook */
>> + BUG_ON(readl(gt_base + GT_CONTROL) &
>> + (GT_CONTROL_COMP_ENABLE |
>> + GT_CONTROL_IRQ_ENABLE |
>> + GT_CONTROL_AUTO_INC));
>
> Here is another one of these checks. Why aren't you just
> zeroing these flags if you don't want them, especially since
> you're writing the register at the end of the function? Now it
> looks like instead of setting up the timer the way you want it
> you bug out if it isn't already set up as you want it (hint,
> the name of this function).
>
>> + /* Fall through */
>> + case CLOCK_EVT_MODE_UNUSED:
>> + case CLOCK_EVT_MODE_SHUTDOWN:
>> + default:
>> + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>
> Why are you enabling the timer in unused and shutdown mode?
>
> This doesn't make sense.
It is because we are using the global-timer block for both clocksource
and clockevents and we do not want the clocksource to disappear when
clockevent is unused/shutdown.
>
>> + break;
>> + }
>> +}
>> +
>> +static int gt_clockevent_set_next_event(unsigned long evt,
>> + struct clock_event_device *unused)
>> +{
>> + gt_compare_set(evt, 0);
>> + return 0;
>> +}
>> +
>> +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
>> +{
>> + struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
>> +
>> + writel(GT_INT_STATUS_EVENT_FLAG, gt_base + GT_INT_STATUS);
>
> You're clearing the flag before checking if it was set. What happens
> if this was a spurious interrupt that should be disregarded?
Makes sense, I will add a check here.
>
>> + evt->event_handler(evt);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int __cpuinit gt_clockevents_init(struct clock_event_device *clk)
>> +{
>> + struct clock_event_device **this_cpu_clk;
>> + int cpu = smp_processor_id();
>> +
>> + clk->name = "Global Timer CE";
>
> Name it after the hardware feature. "ARM global timer clock event"
> there is no need to abbreviate randomly.
Yes, will change all such instances.
>
>> + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>> + clk->set_mode = gt_clockevent_set_mode;
>> + clk->set_next_event = gt_clockevent_set_next_event;
>> + this_cpu_clk = __this_cpu_ptr(gt_evt);
>> + *this_cpu_clk = clk;
>> + clk->irq = gt_ppi;
>> + clockevents_config_and_register(clk, gt_clk_rate,
>> + 0xf, 0xffffffff);
>
> Why can't this clock event handle anything lower than 0xf?
> Does that come from the datasheet or have you just copied some
> code?
There is no such limitation on the minimum clock ticks, I think it was
copied.
Will fix it in next version.
>
> Further, since this clock event hardware *most definately* supports
> using a delta upper bound *beyond* 32 bits, I think the clock event
> core code should be altered to allow for registereing such clock
> events, but TGLX may have some idea here. This will work but will
> not expose the full potential of this 64-bit counter hardware.
>
>> + per_cpu(percpu_init_called, cpu) = true;
>> + enable_percpu_irq(clk->irq, IRQ_TYPE_NONE);
>> + return 0;
>> +}
>> +
>> +static void gt_clockevents_stop(struct clock_event_device *clk)
>> +{
>> + gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
>> + disable_percpu_irq(clk->irq);
>> +}
>> +
>> +static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk)
>> +{
>> + int cpu = smp_processor_id();
>> +
>> + /* Use existing clock_event for boot cpu */
>> + if (per_cpu(percpu_init_called, cpu))
>> + return 0;
>> +
>> + writel(0, gt_base + GT_CONTROL);
>> + writel(0, gt_base + GT_COUNTER0);
>> + writel(0, gt_base + GT_COUNTER1);
>> + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>> +
>> + return gt_clockevents_init(clk);
>> +}
>> +
>> +static cycle_t gt_clocksource_read(struct clocksource *cs)
>> +{
>> + union gt_counter res = gt_counter_read();
>> + return res.cycles;
>> +}
>> +
>> +static struct clocksource gt_clocksource = {
>> + .name = "Global Timer CS",
>
> "ARM global timer clock source"
>
>> + .rating = 300,
>> + .read = gt_clocksource_read,
>> + .mask = CLOCKSOURCE_MASK(64),
>> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
>> +};
>> +
>> +static void __init gt_clocksource_init(void)
>> +{
>> + writel(0, gt_base + GT_CONTROL);
>> + writel(0, gt_base + GT_COUNTER0);
>> + writel(0, gt_base + GT_COUNTER1);
>> + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>> +
>> + gt_clocksource.shift = 20;
>
> So how did you come up with that?
Hmm..
You are right, this should not be a constant here, I will use
clocksource_register_hz instead.
-srini
>
>> + gt_clocksource.mult =
>> + clocksource_hz2mult(gt_clk_rate, gt_clocksource.shift);
>> + clocksource_register(>_clocksource);
>
> Why don't you just replace all of this hazzle with:
>
> clocksource_register_hz(>_clocksource, gt_clk_rate);
>
> That will calculate mult and shift for you? (Leave these
> unassigned.)
>
> Since the hpet timer does this it's pretty well tested...
>
> No more comments right now...
>
> Yours,
> Linus Walleij
>
>
next prev parent reply other threads:[~2013-05-14 8:46 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-08 14:09 [RFC 0/8] ARM:STiH41x: Add STiH41x platform and board support Srinivas KANDAGATLA
[not found] ` <1368022248-2153-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 14:34 ` [RFC 1/8] serial:st-asc: Add ST ASC driver Arnd Bergmann
2013-05-08 14:39 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 18:18 ` Srinivas KANDAGATLA
2013-05-08 19:55 ` Arnd Bergmann
2013-05-08 15:34 ` Greg KH
2013-05-08 15:40 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 15:53 ` Greg KH
2013-05-08 16:03 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 16:15 ` Greg KH
2013-05-08 16:31 ` Arnd Bergmann
2013-05-08 16:36 ` Greg KH
2013-05-10 23:29 ` Russell King - ARM Linux
2013-05-08 16:39 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 16:45 ` Nicolas Pitre
2013-05-08 18:35 ` Arnd Bergmann
2013-05-09 13:36 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 16:10 ` Stephen GALLIMORE
2013-05-10 14:45 ` Ben Dooks
2013-05-10 15:23 ` Greg KH
2013-05-10 15:31 ` Ben Dooks
2013-05-10 15:40 ` Stuart MENEFY
2013-05-08 18:02 ` Srinivas KANDAGATLA
[not found] ` <1368022260-2197-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 14:26 ` [RFC 2/8] ARM:global_timer: Add ARM global timer support Rob Herring
2013-05-08 15:06 ` Stuart MENEFY
2013-05-08 14:38 ` Arnd Bergmann
2013-05-08 14:49 ` Will Deacon
2013-05-08 15:48 ` Stuart MENEFY
2013-05-08 16:23 ` Arnd Bergmann
2013-05-08 14:51 ` Steffen Trumtrar
2013-05-09 14:07 ` Srinivas KANDAGATLA
2013-05-09 14:51 ` Arnd Bergmann
2013-05-09 14:51 ` Srinivas KANDAGATLA
2013-05-13 19:05 ` Linus Walleij
2013-05-13 19:30 ` Thomas Gleixner
2013-05-14 8:46 ` Srinivas KANDAGATLA [this message]
2013-05-14 9:23 ` Linus Walleij
2013-05-14 10:26 ` Srinivas KANDAGATLA
[not found] ` <1368022272-2241-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 14:50 ` [RFC 3/8] mfd:syscon: Introduce claim/read/write/release APIs Arnd Bergmann
2013-05-08 15:01 ` Mark Brown
2013-05-08 17:42 ` Srinivas KANDAGATLA
2013-05-09 9:51 ` Mark Brown
2013-05-09 11:58 ` Srinivas KANDAGATLA
2013-05-09 13:26 ` Mark Brown
2013-05-09 14:00 ` Srinivas KANDAGATLA
2013-05-09 14:40 ` Mark Brown
2013-05-09 14:47 ` Srinivas KANDAGATLA
2013-05-10 12:51 ` Srinivas KANDAGATLA
2013-05-08 17:32 ` Srinivas KANDAGATLA
2013-05-08 19:48 ` Arnd Bergmann
2013-05-09 10:17 ` Srinivas KANDAGATLA
2013-05-17 14:36 ` Arnd Bergmann
2013-05-20 12:48 ` Srinivas KANDAGATLA
2013-05-23 21:44 ` Arnd Bergmann
2013-05-24 16:06 ` Srinivas KANDAGATLA
[not found] ` <1368022284-2283-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 15:06 ` [RFC 4/8] pinctrl:stixxxx: Add pinctrl and pinconf support Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 16:27 ` Srinivas KANDAGATLA
2013-05-08 16:38 ` Jean-Christophe PLAGNIOL-VILLARD
[not found] ` <1368022297-2334-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 16:18 ` [RFC 5/8] ARM:stih41x: Add STiH415 SOC support Arnd Bergmann
2013-05-08 16:21 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 16:50 ` Stephen GALLIMORE
2013-05-08 18:55 ` Arnd Bergmann
2013-05-09 11:09 ` Stephen GALLIMORE
2013-05-08 17:03 ` Srinivas KANDAGATLA
[not found] ` <1368022329-2424-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-08 16:20 ` [RFC 7/8] ARM:stih41x: Add B2000 board support Arnd Bergmann
2013-05-08 16:24 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-08 17:04 ` Srinivas KANDAGATLA
[not found] ` <1368022318-2380-1-git-send-email-srinivas.kandagatla@st.com>
2013-05-15 19:41 ` [RFC 6/8] ARM:stih41x: Add STiH416 SOC support Linus Walleij
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=5191F9EE.6070008@st.com \
--to=srinivas.kandagatla@st$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox