Re: [PATCH v4 21/29] nios2: Time keeping

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 8 Oct 2014, Arnd Bergmann wrote:
> On Wednesday 08 October 2014 17:20:44 Thomas Gleixner wrote:
> > On Wed, 8 Oct 2014, Arnd Bergmann wrote:
> > > On Wednesday 08 October 2014 12:44:32 Thomas Gleixner wrote:
> > > > > +static int num_called;
> > > > > +static void __init nios2_time_init(struct device_node *timer)
> > > > > +{
> > > > > +     switch (num_called) {
> > > > > +     case 0:
> > > > > +             nios2_clockevent_init(timer);
> > > > > +             break;
> > > > > +     case 1:
> > > > > +             nios2_clocksource_init(timer);
> > > > > +             break;
> > > > > +     default:
> > > > > +             break;
> > > > > +     }
> > > > > +
> > > > > +     num_called++;
> > > > > +}
> > > > 
> > > > Eew. So this depends on the DT ordering. If thats wrong, then stuff
> > > > will be initialized in the wrong oder. 
> > > > 
> > > > > +CLOCKSOURCE_OF_DECLARE(nios2_timer, "altr,timer-1.0", nios2_time_init);
> > > > 
> > > > Why can't you have separate match entries with where one calls
> > > > nios2_clockevent_init and the other nios2_clocksource_init?
> > > 
> > > I believe we have the same logic in other drivers as well, the intention
> > > being that if you have multiple identical timers, the first one will
> > > be used as clockevent and the second one (if there is more than one)
> > > becomes the clocksource.
> > > 
> > > If the hardware is really identical, I would argue that the comaptible
> > > string ought to be the same as well, as the DT is not supposed to
> > > care about what the timers are used for in Linux.
> > 
> > So why do we need two calls at all if we have one piece of hardware
> > which has two functions? We know that already, don't we?
> 
> It's not one piece of hardware with two functions, but multiple (n >= 1)
> pieces of identical hardware at different register locations.
> 
> The usual case is that you have one hardware block that may exist in
> any number of instances, but each instance can only be used for
> either clockevent or clocksource but not both.
> 
> If we have only one instance in an FPGA, the code above will use
> it as a clockevent. If there are two or more instances, the second
> one gets used as clocksource and all others are unused, until we
> change the kernel to come up with a third use case.

Ok, got it. It does not change the fact that the above function is
butt ugly and completely non intuitive.

I also do not agree, that the DT should not tell which of the timers
should be used as a clockevent device and which one as a
clocksource. If you have a better suited clockevent device in your
FPGA, you cannot use a single instance of the timer thingy as it will
be registered as a clockevent and not as a clocksource. So explicit
match values for particular hardware instances would be more flexible,
but I don't have strong feelings about it either.

Thanks,

	tglx