Received-SPF: pass (google.com: domain of linux-kernel+bounces-201716-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <TYSPR04MB708410A11D944F4553DB55A08AEB2@TYSPR04MB7084.apcprd04.prod.outlook.com>
In-Reply-To: <TYSPR04MB708410A11D944F4553DB55A08AEB2@TYSPR04MB7084.apcprd04.prod.outlook.com>
From: Chunyan Zhang <zhang.lyra@gmail.com>
Date: Wed, 5 Jun 2024 11:15:59 +0800
Message-ID: <CAAfSe-tDX-2VTJJFpZ8fF+BLbbiOCdHX1+6A9c224KwVaGC_CA@mail.gmail.com>
Subject: Re: [PATCH] clocksource/drivers/sprd: Enable register for timer
 counter from 32 bit to 64 bit
To: Enlin Mu <enlin.mu@outlook.com>
Cc: orsonzhai@gmail.com, baolin.wang@linux.alibaba.com, tglx@linutronix.de, 
	enlin.mu@unisoc.com, enlinmu@gmail.com, linux-kernel@vger.kernel.org, 
	linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Enlin,

On Wed, 22 May 2024 at 16:15, Enlin Mu <enlin.mu@outlook.com> wrote:
>
> From: Enlin Mu <enlin.mu@unisoc.com>
>
> Using 32 bit for suspend compensation, the max compensation time is 36
> hours(working clock is 32k).In some IOT devices, the suspend time may
> be long, even exceeding 36 hours. Therefore, a 64 bit timer counter
> is needed for counting.
>
> Signed-off-by: Enlin Mu <enlin.mu@unisoc.com>
> ---
>  drivers/clocksource/timer-sprd.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c
> index 430cb99d8d79..936691e27f8b 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_HI_MASK    GENMASK(31, 0)
>
>  static void sprd_timer_enable(void __iomem *base, u32 flag)
>  {
> @@ -57,10 +58,11 @@ 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 cycles_lo,
> +                                       unsigned long cycles_hi)

I would suggest using u64 rather than adding a new parameter. In this
way we can avoid some of the changes below.
>  {
> -       writel_relaxed(cycles & TIMER_VALUE_LO_MASK, base + TIMER_LOAD_LO);
> -       writel_relaxed(0, base + TIMER_LOAD_HI);
> +       writel_relaxed(cycles_lo & TIMER_VALUE_LO_MASK, base + TIMER_LOAD_LO);
> +       writel_relaxed(cycles_hi, base + TIMER_LOAD_HI);
>  }
>
>  static void sprd_timer_enable_interrupt(void __iomem *base)
> @@ -82,7 +84,8 @@ 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), cycles,
> +                               (u64)cycles >> 32);

On 32-bit systems, TIMER_LOAD_HI is still 0.

>         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), timer_of_period(to),
> +                               (u64)timer_of_period(to) >> 32);

Same here, so do you need to consider 32-bit systems?

>         sprd_timer_enable(timer_of_base(to), TIMER_CTL_PERIOD_MODE);
>
>         return 0;
> @@ -162,14 +166,21 @@ static struct timer_of suspend_to = {
>
>  static u64 sprd_suspend_timer_read(struct clocksource *cs)
>  {
> -       return ~(u64)readl_relaxed(timer_of_base(&suspend_to) +
> -                                  TIMER_VALUE_SHDW_LO) & cs->mask;
> +       u32 hi, lo;
> +
> +       lo = readl_relaxed(timer_of_base(&suspend_to) +
> +                                  TIMER_VALUE_SHDW_LO);
> +       hi = readl_relaxed(timer_of_base(&suspend_to) +
> +                                  TIMER_VALUE_SHDW_HI);
> +
> +       return ~((u64)hi << 32 | lo);
>  }
>
>  static int sprd_suspend_timer_enable(struct clocksource *cs)
>  {
>         sprd_timer_update_counter(timer_of_base(&suspend_to),
> -                                 TIMER_VALUE_LO_MASK);
> +                                 TIMER_VALUE_LO_MASK,
> +                                 TIMER_VALUE_HI_MASK);

Like I suggested above, not add a new parameter, then pass
CLOCKSOURCE_MASK(64) as the cycles, that would avoid wrongly setting
TIMER_LOAD_HI for 32-bit systems based on the current implementations.

Thanks,
Chunyan

>         sprd_timer_enable(timer_of_base(&suspend_to), TIMER_CTL_PERIOD_MODE);
>
>         return 0;
> @@ -186,7 +197,7 @@ static struct clocksource suspend_clocksource = {
>         .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,
>  };
>
> --
> 2.39.2
>