Messages in this thread | | | From | weidong guan <> | Date | Wed, 5 Jan 2022 10:29:08 +0800 | Subject | Re: [PATCH] clocksource: fix counter with 32bit may wrap around |
| |
On Tue, Jan 4, 2022 at 9:42 AM Baolin Wang <baolin.wang7@gmail.com> wrote: > > Hi, > > On Fri, Dec 31, 2021 at 2:03 PM weidong guan <weidong.guanguan@gmail.com> wrote: > > > > From: "weidong.guan" <weidong.guan@unisoc.com> > > > > When the system sleep time exceeds 30+ hours, counter with 32bit may wrap > > I am curious about which condition the system can suspend for more > than 30+ hours, from my understanding, the phone will be woken up > periodically.
Yes, it does on mobile phones, but on some non-mobile devices, it would continue to sleep until you wake it up actively.Just as logs shown below: [89438.802502][11-09 09:14:34.802] PM: suspend entry (deep) [200927.406098][11-10 16:12:43.406] PM: suspend exit And in my opinion, should we try to avoid counter overflow even if theoretically?
> > > around, resulting in insufficient sleep compensation time. So We should > > enable 64bit counter mode of sprd timer and use the given mult/shift > > to ensure that sufficient counts can be generated. > > > > Signed-off-by: weidong.guan <weidong.guan@unisoc.com> > > --- > > drivers/clocksource/timer-sprd.c | 31 ++++++++++++++++++++----------- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c > > index 430cb99..7eac059 100644 > > --- a/drivers/clocksource/timer-sprd.c > > +++ b/drivers/clocksource/timer-sprd.c > > @@ -30,6 +30,7 @@ > > #define TIMER_VALUE_SHDW_HI 0x1c > > > > #define TIMER_VALUE_LO_MASK GENMASK(31, 0) > > +#define TIMER_VALUE_64BIT_MASK GENMASK_ULL(63, 0) > > > > static void sprd_timer_enable(void __iomem *base, u32 flag) > > { > > @@ -57,10 +58,12 @@ static void sprd_timer_disable(void __iomem *base) > > writel_relaxed(val, base + TIMER_CTL); > > } > > > > -static void sprd_timer_update_counter(void __iomem *base, unsigned long cycles) > > +static void > > +sprd_timer_update_counter(void __iomem *base, unsigned long long cycles) > > { > > writel_relaxed(cycles & TIMER_VALUE_LO_MASK, base + TIMER_LOAD_LO); > > - writel_relaxed(0, base + TIMER_LOAD_HI); > > + writel_relaxed((cycles >> 32) & TIMER_VALUE_LO_MASK, > > + base + TIMER_LOAD_HI); > > } > > > > static void sprd_timer_enable_interrupt(void __iomem *base) > > @@ -82,7 +85,7 @@ static int sprd_timer_set_next_event(unsigned long cycles, > > struct timer_of *to = to_timer_of(ce); > > > > sprd_timer_disable(timer_of_base(to)); > > - sprd_timer_update_counter(timer_of_base(to), cycles); > > + sprd_timer_update_counter(timer_of_base(to), (u64)cycles); > > sprd_timer_enable(timer_of_base(to), 0); > > > > return 0; > > @@ -93,7 +96,8 @@ static int sprd_timer_set_periodic(struct clock_event_device *ce) > > struct timer_of *to = to_timer_of(ce); > > > > sprd_timer_disable(timer_of_base(to)); > > - sprd_timer_update_counter(timer_of_base(to), timer_of_period(to)); > > + sprd_timer_update_counter(timer_of_base(to), > > + (u64)timer_of_period(to)); > > sprd_timer_enable(timer_of_base(to), TIMER_CTL_PERIOD_MODE); > > > > return 0; > > @@ -162,15 +166,19 @@ static int __init sprd_timer_init(struct device_node *np) > > > > static u64 sprd_suspend_timer_read(struct clocksource *cs) > > { > > - return ~(u64)readl_relaxed(timer_of_base(&suspend_to) + > > - TIMER_VALUE_SHDW_LO) & cs->mask; > > + u64 value_hi, value_lo; > > + > > + value_hi = (u64)readl_relaxed(timer_of_base(&suspend_to) + TIMER_VALUE_SHDW_HI); > > + value_lo = (u64)readl_relaxed(timer_of_base(&suspend_to) + TIMER_VALUE_SHDW_LO); > > + return ~(u64)((value_hi << 32) + value_lo) & cs->mask; > > } > > > > static int sprd_suspend_timer_enable(struct clocksource *cs) > > { > > sprd_timer_update_counter(timer_of_base(&suspend_to), > > - TIMER_VALUE_LO_MASK); > > - sprd_timer_enable(timer_of_base(&suspend_to), TIMER_CTL_PERIOD_MODE); > > + TIMER_VALUE_64BIT_MASK); > > + sprd_timer_enable(timer_of_base(&suspend_to), > > + TIMER_CTL_PERIOD_MODE|TIMER_CTL_64BIT_WIDTH); > > > > return 0; > > } > > @@ -183,10 +191,12 @@ static void sprd_suspend_timer_disable(struct clocksource *cs) > > static struct clocksource suspend_clocksource = { > > .name = "sprd_suspend_timer", > > .rating = 200, > > + .mult = 0x1DCD650, > > + .shift = 10, > > And why?
Do you mean why we use the given mult/shift? The reason is If we use the mult/shift automatically calculated by kernel, the mult value is relatively large, causing the not large enough maximum count value. So under the precondition of ensuring accuracy, we calculate a relatively small mult/shift value, aiming to get larger maximum count value.
> > > .read = sprd_suspend_timer_read, > > .enable = sprd_suspend_timer_enable, > > .disable = sprd_suspend_timer_disable, > > - .mask = CLOCKSOURCE_MASK(32), > > + .mask = CLOCKSOURCE_MASK(64), > > .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP, > > }; > > > > @@ -198,8 +208,7 @@ static int __init sprd_suspend_timer_init(struct device_node *np) > > if (ret) > > return ret; > > > > - clocksource_register_hz(&suspend_clocksource, > > - timer_of_rate(&suspend_to)); > > + clocksource_register_hz(&suspend_clocksource, 0); > > -- > Baolin Wang
| |