Date:	Fri, 20 Mar 2009 10:26:33 +0100
From:	Michal Simek <monstr@...str.eu>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	LKML <linux-kernel@...r.kernel.org>, john.williams@...alogix.com,
	John Stultz <johnstul@...ibm.com>
Subject: Re: [PATCH 08/57] microblaze_v7: Interrupt handling, timer support,
 selfmod code

Thomas,

just one other question.
For me will be useful to use second timer which is inside timer IP core.
There are two timers with one interrupt line. And I can of course resolve which
counter cause it. That's no problem.

My question is about timer_irqaction where is dev_id. What should be there?
Point to clocksource structure or clockevent?

static struct irqaction timer_irqaction = {
	.handler = timer_interrupt,
	.flags = IRQF_DISABLED | IRQF_TIMER,
	.name = "timer",
	.dev_id = &clocksource_microblaze,
};

Thanks,
Michal

> 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.
> 
>> 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);
> 
> 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.
> 
> 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.
> 
>> 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.
> 
>>>> +	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. 
>  
>>>> +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 ?
>  
>>>> +	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.
> 
>      tglx


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
--
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