lkml.org 
[lkml]   [2009]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 08/57] microblaze_v7: Interrupt handling, timer support, selfmod code
    Hi Thomas,

    > Michal,
    >
    > On Thu, 19 Mar 2009, Michal Simek wrote:
    >>>> + __do_IRQ(irq);
    >>> You use irq chips now and you set the type handlers (edge/level), but
    >>> you still call __do_IRQ() the all in one fits nothing handler, which
    >>> is going to be deprecated and removed.
    >> I know about.
    >>> Please call
    >>> generic_handle_irq(irq);
    >>>
    >>> which will call the correct flow handlers.
    >> I would like to use it but don't work with edge interrupt.
    >> __do_IRQ handle it in right way.
    >>
    >> I used this implementation but I did some change edge/level handling and I can't
    >> use it.
    >> http://developer.petalogix.com/git/gitweb.cgi?p=linux-2.6-microblaze.git;a=blob_plain;f=arch/microblaze/kernel/irq.c;hb=3645d887ad6443a262bbeddf384038321172db2b
    >>
    >> Any hints what could be wrong?
    >
    > Look at the different handling schemes of __do_IRQ and handle_edge_irq
    > vs. the chip functions:
    >
    > __do_IRQ() does:
    > {
    > chip->ack();
    >
    > handle_IRQ_event();
    >
    > chip->end();
    > }
    >
    > handle_edge_irq() does:
    > {
    > if ((desc->status & (IRQ_INPROGRESS | IRQ_DISABLED)) ||
    > !desc->action)) {
    > desc->status |= (IRQ_PENDING | IRQ_MASKED);
    > mask_ack_irq(desc, irq);
    > goto out_unlock;
    > }
    >
    > chip->ack();
    > handle_IRQ_event();
    >
    > }
    >
    > I guess the problem is in your chip->xxx functions.

    OK. I look at it in detail.
    >
    >> First of all I have one question to you about MB timer.c.
    >> It is about this function - microblaze_read.
    >>
    >> static cycle_t mb_tick_cnt; /* store counter ticks */
    >>
    >> static cycle_t microblaze_read(void)
    >> {
    >> u64 temp = (u64)mb_tick_cnt + (u64)((u32)cpuinfo.freq_div_hz
    >> - (u32)in_be32(TIMER_BASE + TCR0));
    >> return temp;
    >> }
    >>
    >>
    >> MB has 32bit periodic down counter and I need to use u64 value that's why
    >> I do these operation above. cpuinfo.freq_div_hz store freq/HZ value - number of
    >> ticks for 1/HZ. I subtract current timer value + mb_tick_cnt which store number
    >> of count. The problem I have is that gettimeofday LTP test failed on it ->
    >> announce that time is going backward.
    >> Simple returning only mb_tick_cnt pass this test but of course I am losing
    >> information about time till 1/HZ.
    >> Do I have to add any specific amount of time which take counting of it?
    >
    > You do not neeed 64 bit values. The return value is masked with the
    > clocksource->mask anyway. So when your clocksource has less than 64
    > bits it's covered.
    >
    > So if your timer counts down from 0xFFFFFFFF to 0 and wraps around you
    > just need to return (cycle_t) ~(timer->count);

    Counts down from specific value to 0. That specific value and number of tick for
    1/HZ. That mean that I need (cycle_t) (specific_value - timer->count)

    >
    > But I think I know where your real problem is. You use the same timer
    > for both timekeeping and the periodic tick. That's why you can not
    > support one shot mode. I bet the machine has two timers.

    yes. I have no problem to use another counter too. It is FPGA and my timer IP
    support two independent timers. ( Her is the link to doc
    http://www.xilinx.com/support/documentation/ip_documentation/xps_timer.pdf)
    >
    > So the best way to handle this is:
    >
    > Use timer A in free running mode - up or down counting does not
    > matter - for the timekeeping. That way you have an ever increasing
    > monotonic time.
    >
    > Use timer B either for periodic mode or for one shot and all your
    > problems are gone. In periodic mode use autoreload and in one shot
    > mode just follow the instructions of the generic code via the
    > timer_set_next_event() function.

    OK. That mean you timer A as clocksource and the second clock as event device.
    I try to do it.
    >
    >> And the second question is about shift and rating values.
    >> I wrote one message in past http://lkml.org/lkml/2009/1/11/291
    >> Here is the important of part of that message.
    >>
    >> ...
    >>
    >> And the second part is about shift and rating values. Rating is
    >> describe(linux/clocksource.h) and seems to me that should be
    >> corresponded with CONFIG_HZ value,right?
    >> And I found any explanation of shift value -> max value for equation
    >> (2-5) * freq << shift / NSEC_PER_SEC should be for my case still 32bit
    >> number, where (2-5s) are because of NTP
    >
    > @John, can you explain the shift vlaue please ?
    >
    >> The second thing which seems to me weird in comparing with others log I
    >> have seen is .resolution value. Full (proc/timer_lists is below) My
    >> .resolution: 10000000 nsecs which
    >> is 1/HZ in nsec. (On others log I saw 1nsec values). My the lowest
    >> resolution is 1/freq = 8nsec (for 125MHz). Is that OK or not.
    >> ...
    >
    > The 1ns is theoretical and indicates that the kernel has high resolution
    > timer support. Your resolution is just HZ.
    >
    >>>> +static int microblaze_timer_set_next_event(unsigned long delta,
    >>>> + struct clock_event_device *dev)
    >>>> +{
    >>>> + printk(KERN_INFO "next event, delta %x, %d\n", (u32)delta, (u32)delta);
    >>> This should be pr_debug() or do you want to flood the syslog in
    >>> every timer interrupt ?
    >> This not flood the syslog. I don't know why (maybe because of missing ONESHOT)
    >> but this code is never called in periodic mode. But you are right if this
    >> function is called a lot it is necessary to use pr_debug -> but this is not my
    >> case in this implementation.
    >
    > Well. Either you have one shot mode, then better make it work and
    > useable or just remove the one shot support until you figure out how
    > to do it.

    I don't have it - this feature is not there. There is only printk message in
    set_mode function.

    What is the best timer implementation in kernel? (For inspiration)

    >
    >>>> + microblaze_timer_start(delta);
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +static void microblaze_timer_set_mode(enum clock_event_mode mode,
    >>>> + struct clock_event_device *evt)
    >>>> +{
    >>>> + microblaze_timer_stop();
    >>>> +
    >>>> + switch (mode) {
    >>>> + case CLOCK_EVT_MODE_PERIODIC:
    >>>> + printk(KERN_INFO "%s: periodic\n", __func__);
    >>> pr_debug as well. That's not very informative
    >> It is only information that timer work in periodic mode.
    >>
    >> Part of kernel log which is there - nothing more.
    >>
    >> microblaze_timer_set_mode: shutdown
    >> microblaze_timer_set_mode: periodic
    >
    > Nothing a normal user is interested in I guess, but ok.

    ok

    >
    >>>> +static irqreturn_t timer_interrupt(int irq, void *dev_id)
    >>>> +{
    >>>> + struct clock_event_device *evt = &clockevent_microblaze_timer;
    >>>> +#ifdef CONFIG_HEART_BEAT
    >>>> + heartbeat();
    >>>> +#endif
    >>>> + timer_ack();
    >>>> +
    >>>> + mb_tick_cnt += cpuinfo.freq_div_hz;
    >>> Hmm. How does that work with oneshot timers ?
    >> Oneshot is not supported yet - only periodic mode. I had to add it mb_tick_cnt
    >> counting because
    >> I don't know reason but without it ( kernel and timer in periodic mode )not
    >> update system time.
    >
    > I don't know how that timer works. Do you have a pointer to hardware
    > docs + chapter reference ?

    Look at link above. Look at programming model chapter.

    >
    >>>> + xtime.tv_sec = mktime(2007, 1, 1, 0, 0, 0);
    >>>> + xtime.tv_nsec = 0;
    >>>> + set_normalized_timespec(&wall_to_monotonic,
    >>>> + -xtime.tv_sec, -xtime.tv_nsec);
    >>> Yuck. What's that ? wall_to_monotonic is maintained by the generic
    >>> code.
    >> I take this part of code from arch/blackfin/kernel/time-ts.c:217-219.
    >> arch/x86/xen/time.c use it too. And arch/arm/kernel/time.c use similar
    >> implemetation.
    >
    > Right. All of them are similar nonsense. If we want a 1/1/2007 base
    > date if there is no RTC which tells us the real date/time then we
    > should do this in the generic code and not implement 10 variations all
    > over the place.

    I have no problem to start from 1970 and remove this part of code. Do nothing
    important to me.

    Thanks,
    Michal

    >
    > tglx


    --
    Michal Simek, Ing. (M.Eng)
    w: www.monstr.eu p: +42-0-721842854


    \
     
     \ /
      Last update: 2009-03-20 07:41    [W:0.042 / U:3.272 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site