Re: [RFC PATCHv3 4/6] clocksource: Add TI-Nspire timer drivers

[Linus Walleij]

On Sun, May 12, 2013 at 6:22 AM, Daniel Tang <dt.tangr@xxxxxxxxx> wrote:

> Signed-off-by: Daniel Tang <dt.tangr@xxxxxxxxx>

A commit message does not hurt.

>  drivers/clocksource/Makefile               |   1 +
>  drivers/clocksource/nspire-classic-timer.c | 199 +++++++++++++++++++++++++++++

This subsystem is managed by Thomas Gleixner and John Stultz so you
should include
them on the To: line in reviewes.

> +++ b/drivers/clocksource/nspire-classic-timer.c
(...)
> +struct nspire_timer {
> +       void __iomem *base;
> +       void __iomem *timer1, *timer2;
> +       void __iomem *interrupt_regs;
> +
> +       int irqnr;
> +
> +       struct clk *clk;
> +       struct clock_event_device clkevt;
> +       struct irqaction clkevt_irq;
> +
> +       char clocksource_name[64];
> +       char clockevent_name[64];
> +};
> +
> +static int nspire_timer_set_event(unsigned long delta,
> +               struct clock_event_device *dev)
> +{
> +       unsigned long flags;
> +       struct nspire_timer *timer = container_of(dev,
> +                       struct nspire_timer,
> +                       clkevt);
> +
> +       local_irq_save(flags);
> +
> +       writel(delta, timer->timer1 + IO_CURRENT_VAL);
> +       writel(CNTL_RUN_TIMER | CNTL_DEC | CNTL_MATCH1,
> +                       timer->timer1 + IO_CONTROL);
> +
> +       local_irq_restore(flags);
> +
> +       return 0;
> +}
> +static void nspire_timer_set_mode(enum clock_event_mode mode,
> +               struct clock_event_device *evt)
> +{
> +       evt->mode = mode;

So you support any mode?

Atleast handle CLOCK_EVT_MODE_SHUTDOWN and CLOCK_EVT_UNUSED
like any well-behaved clock event driver.

> +}
> +
> +static irqreturn_t nspire_timer_interrupt(int irq, void *dev_id)
> +{
> +       struct nspire_timer *timer = dev_id;
> +
> +       writel((1<<0), timer->interrupt_regs + IO_INTR_ACK);

write(0x1, timer->interrupt_regs + IO_INTR_ACK);

is no less intelligible, if you really want to point out that you're
setting bit 0, include <linux/bitops.h> and use BIT(0).

If there is a status bit you're clearing, maybe you should check
it first to watch for spurious IRQs?

> +       writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL);
> +
> +       if (timer->clkevt.event_handler)
> +               timer->clkevt.event_handler(&timer->clkevt);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int __init nspire_timer_add(struct device_node *node)
> +{
> +       struct nspire_timer *timer;
> +       struct resource res;
> +       int ret;
> +
> +       timer = kzalloc(sizeof(*timer), GFP_ATOMIC);
> +       if (!timer)
> +               return -ENOMEM;
> +
> +       timer->base = of_iomap(node, 0);
> +       if (!timer->base) {
> +               ret = -EINVAL;
> +               goto error_free;
> +       }
> +       timer->timer1 = timer->base + IO_TIMER1;
> +       timer->timer2 = timer->base + IO_TIMER2;
> +
> +       timer->clk = of_clk_get(node, 0);
> +       if (IS_ERR(timer->clk)) {
> +               ret = PTR_ERR(timer->clk);
> +               pr_err("Timer clock not found! (error %d)\n", ret);
> +               goto error_unmap;
> +       }
> +
> +       timer->interrupt_regs = of_iomap(node, 1);

Check for errors.

> +       timer->irqnr = irq_of_parse_and_map(node, 0);

Check for errors.

> +
> +       of_address_to_resource(node, 0, &res);
> +       scnprintf(timer->clocksource_name, sizeof(timer->clocksource_name),
> +                       "%llx.%s_clocksource",
> +                       (unsigned long long)res.start, node->name);
> +
> +       scnprintf(timer->clockevent_name, sizeof(timer->clockevent_name),
> +                       "%llx.%s_clockevent",
> +                       (unsigned long long)res.start, node->name);
> +
> +       if (timer->interrupt_regs && timer->irqnr) {
> +               timer->clkevt.name      = timer->clockevent_name;
> +               timer->clkevt.set_next_event = nspire_timer_set_event;
> +               timer->clkevt.set_mode  = nspire_timer_set_mode;
> +               timer->clkevt.rating    = 200;
> +               timer->clkevt.shift     = 32;

Delete this shift. It will be set by clockevents_config_and_register().

> +               timer->clkevt.cpumask   = cpu_all_mask;
> +               timer->clkevt.features  =
> +                       CLOCK_EVT_FEAT_ONESHOT;
> +
> +               writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL);
> +               writel(0, timer->timer1 + IO_DIVIDER);

For a 16-bit counter I don't think it's wise to set the divider to 0
unless the clock frequency is very low. See further comments
below. (Assumes this is some prescaler.)

> +               /* Mask out interrupts except the one we're using */
> +               writel((1<<0), timer->interrupt_regs + IO_INTR_MSK);

Just 0x1 or BIT(0).

> +               writel(0x3F, timer->interrupt_regs + IO_INTR_ACK);

Hm magic number, I guess it's clearing all IRQ lines...

> +
> +               /* Interrupt to occur when timer value matches 0 */
> +               writel(0, timer->base + IO_MATCH1);
> +
> +               timer->clkevt_irq.name  = timer->clockevent_name;
> +               timer->clkevt_irq.handler = nspire_timer_interrupt;
> +               timer->clkevt_irq.dev_id = timer;
> +               timer->clkevt_irq.flags =
> +                       IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL;
> +
> +               setup_irq(timer->irqnr, &timer->clkevt_irq);
> +
> +               clockevents_config_and_register(&timer->clkevt,
> +                               clk_get_rate(timer->clk), 0x0001, 0xfffe);
> +               pr_info("Added %s as clockevent\n", timer->clockevent_name);
> +       }
> +
> +       writel(CNTL_RUN_TIMER | CNTL_FOREVER | CNTL_INC,
> +                       timer->timer2 + IO_CONTROL);
> +
> +       clocksource_mmio_init(timer->timer2 + IO_CURRENT_VAL,
> +                       timer->clocksource_name,
> +                       clk_get_rate(timer->clk),
> +                       200, 16,
> +                       clocksource_mmio_readw_up);

If this timer is really just 16 bits, it's a *pretty* good idea to use
the prescaler (I guess this is what IO_DIVIDER is) beacuse else you
will get short sleep times with CONFIG_NO_HZ_IDLE on this system,
and wake up unnecessarily often.

The same goes for the clock event.

See how we calculate the prescaler in arch/arm/mach-integrator/integrator_ap.c
for example.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/