Re: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver

[Thomas Gleixner]

On Thu, 28 Jul 2016, Rich Felker wrote:
> On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > > +	return ((u64)sechi << 32 | seclo) * NSEC_PER_SEC + nsec;
> > 
> > Wow, that's an expensive thing for a hotpath operation. You really don't have
> > binary readout register for that clock thingy?
> 
> Unforunately the clock is in sec64.nsec32 format instead of a flat
> nanoseconds count. Daniel Lezcano also suggested just using
> nanoseconds, which would still need some retry and arithmetic for
> adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps
> at NSEC_PER_SEC rather than at a power of two) but that does put a
> hard upper bound on tickless sleep time of 4 sec. In practice it
> probably doesn't matter. Should I try that instead?

Up to you. I was just wondering about the MUL.
 
> > > +static notrace u64 jcore_sched_clock_read(void)
> > > +{
> > > +	return jcore_clocksource_read(jcore_cs);
> > 
> > Why don't you stuff the above code into this function?
> 
> I was trying to avoid code duplication, but I could if you think it
> really matters for performance.

Did not see where the other usage site was. Must have missed that. So keep it
as is and please add notrace to jcore_clocksource_read().
 
> > Please convert this to the new state machine model of cpu
> > hotplug. CPU_STARTING will be gone very soon. Here is an example:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=smp/hotplug&id=7e86e8bd8dd67649d176e08d8dfb90039f0a1e98
> 
> I don't think that's the commit you wanted to link -- it doesn't show
> a usage example.
 
It's the conversion of a driver from the old to the new style, So it's an
example how to move your stuff to the new interface or am I missing something
here?

> I've asked about this before in another context but didn't get an
> answer -- I'm a bit concerned that, from what I can tell, the new
> framework is a big singleton does assumes singletons in all the
> drivers that use it. In practice it doesn't matter as long as there's
> only one instance of the pit driver, but this seems architecturally
> really bad and like it's a time bomb waiting to blow up on us in the
> future. Am I missing something?

Most of the users are single instance. We have a dynamic interface for the
online callbacks and we might get one for the prep stage as well.

Now for the real core stuff like starting/dying we want explicit states and if
a driver really is multi instance then having a private list there is not
rocket science. Most of them have an instance list anyway.
 
Thanks,

	tglx