EC: Refactor timer code to properly handle non 1MHz hardware timers. |
|
Issue descriptionBasically, a lot of our code assumes that hw_clock_source_read() returns a full-range 32-bit value, not something that wraps before 0xFFFFFFFF. So, say you wrap at 0x0aaaaaaa because CLOCK_FREQ is 24 MHz. If you start udelay(10000) at 0x0aaaaaa0, then 10 ticks later wrap to 0x00000000, udelay will think that roughly 4000 seconds have passed and will exit. We could make __hw_clock_source_read keep its own 32-bit accumulator. But it's probably better just to add support to common/timer.c to handle the case where __hw_clock_source_read() returns something not microseconds (could be a 24MHz clock, could be a 262KHz clock), because we've run into this on several chips now. And then make sure that ONLY timer.c calls __hw_clock_source_read().
,
Oct 11 2016
That's not a bug ... but a feature. we decided long time ago (5 years ?) that to keep the timer code small and fast (including not doing divisions on the critical path), we would use chips having a microseconds hardware counter. This still doesn't seem a crazy requirement on a modern microcontroller in 2016. e.g on a Cortex-M0, the udelay latency in this CL : https://chromium-review.googlesource.com/#/c/395635/1/common/timer.c looks at least one order of magnitude worse than the previous version.
,
Oct 11 2016
This is a substantial enough change that it should have a design doc + review. I think there are 3 cases we need to consider. The case for each board should be known at compile-time, so we don't need to generate less-efficient code on platforms which do have a ~1 MHz timer. 1) Chip has a ~1 MHz timer. Do what we do now; scaling code is not even compiled. 2) Timer is > 1 MHz. Delays should be converted to ticks (one multiply; no divides), which is both more computationally efficient and more accurate. If we're lucky (as on IT83xx = 8 MHz), it's a power of 2 off and we can just shift down to get 1 MHz. Need to have an accumulator, since the timer range isn't 2^32 * 1 MHz long. Consider approximate multiple-and-shift for scaling, rather than divide, with constants determined at compile time rather than being computed on the fly via clock_get_freq() or similar. 3) Timer is < 1 MHz. Similar issues, plus we need to make sure that delays round up. That is, if we have a 100 KHz clock, udelay(9) should delay for ~1 tick, not ~0 ticks.
,
Oct 12 2016
Re#3 Case(1) and (3) can be solved easily. However, case (2) is more tricky. I move some calculation to compile time but I still need to do a division on the fly for case(2). (https://chromium-review.googlesource.com/#/c/395635/) If we want to convert the delay from us to ticks to avoid division, it will involve a significant code change.
,
Oct 13 2016
> Case(1) and (3) can be solved easily. for case (1), the current CL is still adding one indirection. |
|
►
Sign in to add a comment |
|
Comment 1 by aaboagye@chromium.org
, Oct 7 2016Owner: philipchen@chromium.org