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

From: 武彦 <wuyan@allwinnertech•com>
To: "Maxime Ripard" <maxime@cerno•tech>
Cc: "daniel.lezcano" <daniel.lezcano@linaro•org>,
	linux-kernel <linux-kernel@vger•kernel.org>, wens <wens@csie•org>,
	黄烁生 <huangshuosheng@allwinnertech•com>, tglx <tglx@linutronix•de>,
	linux-arm-kernel <linux-arm-kernel@lists•infradead.org>
Subject: Re: Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping
Date: Sat, 12 Dec 2020 19:14:26 +0800	[thread overview]
Message-ID: <2020121219142521192596@allwinnertech.com> (raw)
In-Reply-To: 20201019093146.purztwtytlozva3t@gilmour.lan

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