* [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.