Message ID | 20201010104603.26646-1-wuyan@allwinnertech.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping | expand |
Hi! On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote: > Signed-off-by: wuyan <wuyan@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
Hi Maxime, Sorry for the delay... On Mon, Oct 19 2020 at 11:31:46 +0200, Maxime Ripard wrote: >Hi! > >On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote: >> Signed-off-by: wuyan <wuyan@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. OK. I'll add the commit log and change my name back to 'Martin Wu' next time. Sorry for the inconvenience. >> Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7 > >This should be removed OK. Thanks for your notice. >> --- >> 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. OK. I'll rename 'regs_offset' to 'sun4i_timer_regs_offset'. >> + TIMER_IRQ_EN_REG, >> + TIMER_IRQ_ST_REG, > >Why do you need to save the interrupt status register? The TIMER_IRQ_ST_REG should not be restored. I'll remove this one. >> + 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. OK. I'll attach 'regs_backup[]' to '(struct timer_of).private_data'. >> /* >> * 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. OK. I'll use 'sun4i_timer_save_regs/sun4i_timer_restore_regs' instead. >> 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. Yes. I'll commit it separately. >Thanks! >Maxime Thanks for your kindly notice. Best Regards, Martin Wu
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[] = { + TIMER_IRQ_EN_REG, + TIMER_IRQ_ST_REG, + 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)]; + /* * 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]); +} + 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); } static irqreturn_t sun4i_timer_interrupt(int irq, void *dev_id) @@ -150,7 +191,7 @@ static struct timer_of to = { .set_state_shutdown = sun4i_clkevt_shutdown, .set_state_periodic = sun4i_clkevt_set_periodic, .set_state_oneshot = sun4i_clkevt_set_oneshot, - .tick_resume = sun4i_clkevt_shutdown, + .tick_resume = sun4i_tick_resume, .set_next_event = sun4i_clkevt_next_event, .cpumask = cpu_possible_mask, },
Signed-off-by: wuyan <wuyan@allwinnertech.com> Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7 --- drivers/clocksource/timer-sun4i.c | 45 +++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-)