* [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
@ 2020-10-10 10:46 wuyan
2020-10-19 9:31 ` Maxime Ripard
0 siblings, 1 reply; 4+ messages in thread
From: wuyan @ 2020-10-10 10:46 UTC (permalink / raw)
To: daniel.lezcano, tglx, mripard, wens
Cc: wuyan, frank, linux-kernel, linux-arm-kernel
Signed-off-by: wuyan <wuyan@allwinnertech.com>
Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7
---
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[] = {
+ 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,
},
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
2020-10-10 10:46 [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping wuyan
@ 2020-10-19 9:31 ` Maxime Ripard
0 siblings, 0 replies; 4+ messages in thread
From: Maxime Ripard @ 2020-10-19 9:31 UTC (permalink / raw)
To: wuyan; +Cc: daniel.lezcano, tglx, wens, linux-kernel, linux-arm-kernel, frank
[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
@ 2020-10-19 9:31 ` Maxime Ripard
0 siblings, 0 replies; 4+ messages in thread
From: Maxime Ripard @ 2020-10-19 9:31 UTC (permalink / raw)
To: wuyan; +Cc: daniel.lezcano, linux-kernel, wens, frank, tglx, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3416 bytes --]
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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
2020-10-19 9:31 ` Maxime Ripard
(?)
@ 2020-12-12 11:14 ` 武彦
-1 siblings, 0 replies; 4+ messages in thread
From: 武彦 @ 2020-12-12 11:14 UTC (permalink / raw)
To: Maxime Ripard
Cc: daniel.lezcano, linux-kernel, wens, 黄烁生,
tglx, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-12 11:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10 10:46 [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping wuyan
2020-10-19 9:31 ` Maxime Ripard
2020-10-19 9:31 ` Maxime Ripard
2020-12-12 11:14 ` 武彦
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.