Hi! On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote: > Signed-off-by: wuyan <wu...@allwinnertech.com>
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
signature.asc
Description: PGP signature
