Re: [RFC PATCH 0/5] Add support for S3 non-stop TSC support.

[John Stultz <john.stultz@linaro.org>]

On 01/22/2013 05:37 PM, Jason Gunthorpe wrote:
> On Tue, Jan 22, 2013 at 04:41:58PM -0800, John Stultz wrote:
>
>> Right but to calculate an suspend interval (since they are likely
>> many orders of magnitude larger then the intervals between timer
>> interrupts), you need different mult/shift selection.  Its splitting
>> out the mult/shift management into a per-subsystem level that is the
> You are talking about overflow in cyclecounter_cyc2ns and the like
> right? The 64 bit cycle_t and the underlying hw counter (eg 64 bit
> rdtsc) are not going to overflow..
>
> An alternate version of cyclecounter_cyc2ns for use by the suspend
> code that handles overflow during the mult/shift operation solves that
> problem:
>
> // Drops some small precision along the way but is simple..
> static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
>                                            cycle_t cycles)
> {
>      u64 max = U64_MAX/cc->mult;
>      u64 num = cycles/max;
>      u64 result = num * ((max * cc->mult) >> cc->shift);
>      return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
> }
>
> Or am I missing the issue?

Well, cyclecounters and clocksources are currently different things. 
There was some hope that cyclecounters would be a simpler base structure 
that would supersede clocksources, but the complexity of all the 
variants of clocksources have limited the ability to make such a 
conversion. At least so far. I hope to eventually clean that up as the 
potential overlap is obvious - although as the 
cyclecounters/timecounters code never grew as I expected. But I'm not 
sure how soon "eventually" will end up being.

But regardless of historical tangents :), you're right, an alternate and 
slower cyc2ns function could be used to avoid overflow issues.


>
>> complicated part. Additionally, there may be cases where the
>> timekeeping clocksource is one clocksource and the suspend
>> clocksource is another. So I think to properly integrate this sort
> Does the difference matter? The clocksource to use is detected at
> runtime, if the timekeeping clocksource is no good for suspend time
> keeping then it just won't be used. With a distinct
> read_persistent_clock API then they are already seperate??

Not sure I'm following you here.  I still think the selection of which 
clocksource to use for suspend timing is problematic (especially if its 
not the active timekeeping clocksource).  So I think instead of 
complicating the generic timekeeping code with the logic, I'd rather 
push it off to new read_presistent_clock api.

thanks
-john