Date:	Tue, 9 Jun 2009 10:51:13 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	liqin.chen@...plusct.com
cc:	linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	torvalds@...ux-foundation.org
Subject: Re: [PATCH 22/27] score: create kernel files signal.c sys_score.c
 time.c

Chen,

On Tue, 9 Jun 2009, liqin.chen@...plusct.com wrote:

> +static struct irqaction timer_irq = {
> +       .handler = timer_interrupt,
> +       .flags = IRQF_DISABLED | IRQF_TIMER,
> +       .mask = CPU_MASK_NONE,
> +       .name = "timer",
> +};
> +
> +static int comparator_next_event(unsigned long delta,
> +               struct clock_event_device *evdev)
> +{
> +       unsigned long   flags;
> +
> +       raw_local_irq_save(flags);


  This function is always called with interrupts disabled.

> +       __raw_writel((TMR_M_PERIODIC | TMR_IE_ENABLE), (void *)P_TIMER0_CTRL);

  Can you please move the type cast to the define or better have a:

  static const __iomem void *timer_ctrl = .....;

  somehwere at the top of the file/

> +       __raw_writel(delta, (void *)P_TIMER0_PRELOAD);
> +       __raw_writel(__raw_readl((void *)P_TIMER0_CTRL) | TMR_ENABLE,
> +               (void *)P_TIMER0_CTRL);
> +
> +       raw_local_irq_restore(flags);
> +
> +       return 0;
> +}
> +
> +static void comparator_mode(enum clock_event_mode mode,
> +               struct clock_event_device *evdev)
> +{
> +       unsigned long   flags;
> +
> +       switch (mode) {
> +       case CLOCK_EVT_MODE_PERIODIC:
> +               raw_local_irq_save(flags);

  This function is also called with interrupts disabled.


> +               __raw_writel((TMR_M_PERIODIC | TMR_IE_ENABLE),
> +                       (void *)P_TIMER0_CTRL);
> +               __raw_writel(SYSTEM_CLOCK/100, (void *)P_TIMER0_PRELOAD);

  shouldn't 100 be replaced by HZ ?

> +               __raw_writel(__raw_readl((void *)P_TIMER0_CTRL) | 
> TMR_ENABLE,
> +                       (void *)P_TIMER0_CTRL);
> +               raw_local_irq_restore(flags);
> +               break;
> +       case CLOCK_EVT_MODE_ONESHOT:
> +       case CLOCK_EVT_MODE_RESUME:
> +       case CLOCK_EVT_MODE_UNUSED:
> +       case CLOCK_EVT_MODE_SHUTDOWN:
> +               break;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static struct clock_event_device comparator = {
> +       .name           = "avr32_comparator",
> +       .features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +       .shift          = 16,
> +       .set_next_event = comparator_next_event,
> +       .set_mode       = comparator_mode,
> +};
> +
> +static u32 score_tick_cnt;
> +
> +static cycle_t score_read_clk(struct clocksource *cs)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +       score_tick_cnt += SYSTEM_CLOCK/100;
> +       local_irq_restore(flags);

  How is that supposed to work ? read_clk() is called from various
  places in the kernel and you seem to add some constant value to it
  on every call. That can not work at all. You need a counter which
  is incrementing at a constant rate and can be read out.

  If you do not have a continous counter then you should not provide a
  clock source. The generic code will then provide the jiffies clock
  source. In that case you need to disable the oneshot mode of your
  clock event device as well.

> +
> +       return score_tick_cnt;
> +}
> +
> +static struct clocksource score_clk = {
> +       .name   = "timer",
> +       .rating = 250,
> +       .read   = score_read_clk,
> +       .shift  = 20,
> +       .mask   = CLOCKSOURCE_MASK(32),
> +       .flags  = CLOCK_SOURCE_IS_CONTINUOUS,

  yeah, continuously wrong :)

> +};
> +
> +void __init time_init(void)
> +{
> +       xtime.tv_sec = mktime(2009, 1, 1, 0, 0, 0);
> +       xtime.tv_nsec = 0;
> +
> +       set_normalized_timespec(&wall_to_monotonic, -xtime.tv_sec,
> +                               -xtime.tv_nsec);

  xtime setup is done in the generic time keeping code already. Please
  remove.

> +       /* disable timer, timer interrupt enable */
> +       __raw_writel((TMR_M_PERIODIC | TMR_IE_ENABLE), (void 
> *)P_TIMER0_CTRL);
> +       __raw_writel(SYSTEM_CLOCK/100, (void *)P_TIMER0_PRELOAD); /* 10ms 
> */
> +
> +       /* start timer */
> +       __raw_writel(__raw_readl((void *)P_TIMER0_CTRL) | TMR_ENABLE,
> +               (void *)P_TIMER0_CTRL);

  No need to start the timer here. When you register your clock event
  device the set_mode function is called and the timer is started.

  Your implementation is not going to work at all.

  A clock event device is a device which delivers either periodic or
  one shot interrupts. It is registered with the generic clockevents
  layer which takes care of setting up the device via the set_mode
  call back.

  The clock source device is a device which provides a monotonic
  increasing counter which can wrap around at some point. The wrap
  around point has to be power of 2 and is defined via the .mask
  member of the clock source device structure. The generic time
  keeping code takes care of the conversion to nsec based time and
  handles NTP and other adjustments.

  So in practice you need either two hardware devices (counter and
  interrupt generator) or a device which has a monotonic increasing
  counter and a match register which generates an interrupt when the
  counter value and the match register are the same.

Thanks,

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

Powered by blists - more mailing lists