Messages in this thread | | | Date | Tue, 16 Dec 2014 13:33:24 -0600 | From | Nishanth Menon <> | Subject | Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856. |
| |
On 12/16/2014 01:27 PM, Lennart Sorensen wrote: >> I see why arch_timer_freq might skip the rounding error of 39, 15 and >> 55 Vs existing logic which is possibly at a truncation error risk >> (without errata for sysclk 13, 26 and 27MHz). >> >> all you'd probably need to do is cast rate, num and den to unsigned >> long and have a common computation logic. > > If that is acceptable, then sure I can do that. I liked avoiding casts > in general though. > >> if you'd really want to handle truncation error, it must be a separate >> patch of it's own - I would not mix it with the errata fix. > > Well there is no error in the existing code because the rate / den > is always a clean integer division. The problem is introduced by the
key is "there is no error in existing code for existing value" :) -> the same code for new values will fail. and introducing (rate * num) / den without cast will fail for old values.
> SYSCLK1 / 610 used by the emulated clock which is not a clean division. > > So for the existing logic, the calculation was perfect. It is only for > the errata case that it is a problem. > > So I think leaving the existing calculation but moved up works well, > and then having the alternate order calculation only in the errata case > seemed cleaner and avoids casts and 64bit math which I thought was > overall desirable.
In general using DIV_ROUND_UP and family of macros(in kernel.h) is the right way of doing division in similar cases in Linux kernel. And for the same functionality, we want a common equation - if it does not fit well for all values (even if we introduce new values), then we must come to a better equation that will work for all values. What we do not do is to have two equations meant for doing the same thing.
-- Regards, Nishanth Menon
| |