Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping

[Maxime Ripard]

Hi!

On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote:
> Signed-off-by: wuyan <wuyan@xxxxxxxxxxxxxxxxx>

A commit log would be welcome here. Also, the last time you contributed
you used the name Martin Wu in your Signed-off-by, it would be nice to
be consistent there.

> Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7

This should be removed

> ---
>  drivers/clocksource/timer-sun4i.c | 45 +++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-sun4i.c b/drivers/clocksource/timer-sun4i.c
> index 0ba8155b8287..49fb6b90ec15 100644
> --- a/drivers/clocksource/timer-sun4i.c
> +++ b/drivers/clocksource/timer-sun4i.c
> @@ -29,6 +29,7 @@
>  #define TIMER_IRQ_EN_REG	0x00
>  #define TIMER_IRQ_EN(val)		BIT(val)
>  #define TIMER_IRQ_ST_REG	0x04
> +#define TIMER_IRQ_CLEAR(val)		BIT(val)
>  #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
>  #define TIMER_CTL_ENABLE		BIT(0)
>  #define TIMER_CTL_RELOAD		BIT(1)
> @@ -41,6 +42,19 @@
>  
>  #define TIMER_SYNC_TICKS	3
>  
> +/* Registers which needs to be saved and restored before and after sleeping */
> +static u32 regs_offset[] = {

It would be better to have a prefix (like sun4i_timer to be consistent)
there so that we know it's less confusing and we know it's not some
generic thing.

> +	TIMER_IRQ_EN_REG,
> +	TIMER_IRQ_ST_REG,

Why do you need to save the interrupt status register?

> +	TIMER_CTL_REG(0),
> +	TIMER_INTVAL_REG(0),
> +	TIMER_CNTVAL_REG(0),
> +	TIMER_CTL_REG(1),
> +	TIMER_INTVAL_REG(1),
> +	TIMER_CNTVAL_REG(1),
> +};
> +static u32 regs_backup[ARRAY_SIZE(regs_offset)];

We should store this one in the timer_of struct so that we don't have
any issue if there's two timers at some point.

>  /*
>   * When we disable a timer, we need to wait at least for 2 cycles of
>   * the timer source clock. We will use for that the clocksource timer
> @@ -82,10 +96,37 @@ static void sun4i_clkevt_time_start(void __iomem *base, u8 timer,
>  	       base + TIMER_CTL_REG(timer));
>  }
>  
> +static inline void save_regs(void __iomem *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> +		regs_backup[i] = readl(base + regs_offset[i]);
> +}
> +
> +static inline void restore_regs(void __iomem *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> +		writel(regs_backup[i], base + regs_offset[i]);
> +}
> +

Same thing here, using the prefix would be nice for those two functions
name.

>  static int sun4i_clkevt_shutdown(struct clock_event_device *evt)
>  {
>  	struct timer_of *to = to_timer_of(evt);
>  
> +	save_regs(timer_of_base(to));
> +	sun4i_clkevt_time_stop(timer_of_base(to), 0);
> +
> +	return 0;
> +}
> +
> +static int sun4i_tick_resume(struct clock_event_device *evt)
> +{
> +	struct timer_of *to = to_timer_of(evt);
> +
> +	restore_regs(timer_of_base(to));
>  	sun4i_clkevt_time_stop(timer_of_base(to), 0);
>  
>  	return 0;
> @@ -126,7 +167,7 @@ static int sun4i_clkevt_next_event(unsigned long evt,
>  
>  static void sun4i_timer_clear_interrupt(void __iomem *base)
>  {
> -	writel(TIMER_IRQ_EN(0), base + TIMER_IRQ_ST_REG);
> +	writel(TIMER_IRQ_CLEAR(0), base + TIMER_IRQ_ST_REG);
>  }

This is mostly a cosmetic change right? Either way, it should be in a
separate patch.

Thanks!
Maxime

Attachment: signature.asc
Description: PGP signature