* [PATCH] clocksource: fix counter with 32bit may wrap around @ 2021-12-31 6:02 weidong guan 2022-01-04 1:43 ` Baolin Wang 0 siblings, 1 reply; 3+ messages in thread From: weidong guan @ 2021-12-31 6:02 UTC (permalink / raw) To: daniel.lezcano Cc: tglx, orsonzhai, baolin.wang7, zhang.lyra, linux-kernel, weidong.guan From: "weidong.guan" <weidong.guan@unisoc.com> When the system sleep time exceeds 30+ hours, counter with 32bit may wrap 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, .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); return 0; } -- 1.9.1 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] clocksource: fix counter with 32bit may wrap around 2021-12-31 6:02 [PATCH] clocksource: fix counter with 32bit may wrap around weidong guan @ 2022-01-04 1:43 ` Baolin Wang 2022-01-05 2:29 ` weidong guan 0 siblings, 1 reply; 3+ messages in thread From: Baolin Wang @ 2022-01-04 1:43 UTC (permalink / raw) To: weidong guan Cc: Daniel Lezcano, Thomas Gleixner, Orson Zhai, Chunyan Zhang, LKML, weidong.guan 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. > 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? > .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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] clocksource: fix counter with 32bit may wrap around 2022-01-04 1:43 ` Baolin Wang @ 2022-01-05 2:29 ` weidong guan 0 siblings, 0 replies; 3+ messages in thread From: weidong guan @ 2022-01-05 2:29 UTC (permalink / raw) To: Baolin Wang Cc: Daniel Lezcano, Thomas Gleixner, Orson Zhai, Chunyan Zhang, LKML, weidong.guan 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-05 2:29 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-31 6:02 [PATCH] clocksource: fix counter with 32bit may wrap around weidong guan 2022-01-04 1:43 ` Baolin Wang 2022-01-05 2:29 ` weidong guan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®