Re: [PATCH V5 2/7] clocksource: tegra: add Tegra210 timer support

[Jon Hunter]

On 01/02/2019 14:39, Joseph Lo wrote:
> On 2/1/19 8:44 PM, Jon Hunter wrote:
>>
>> On 01/02/2019 03:36, Joseph Lo wrote:
>>> Add support for the Tegra210 timer that runs at oscillator clock
>>> (TMR10-TMR13). We need these timers to work as clock event device and to
>>> replace the ARMv8 architected timer due to it can't survive across the
>>> power cycle of the CPU core or CPUPORESET signal. So it can't be a
>>> wake-up
>>> source when CPU suspends in power down state.
>>>
>>> Also convert the original driver to use timer-of API.
>>
>> It may have been nice to split this into 2 patches to make it easier to
>> see what is going on but not a big deal.
>>
>>> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
>>> Acked-by: Thierry Reding <treding@xxxxxxxxxx>
>>> ---
>>> v5:
>>> Â * add ack tag from Thierry
>>> v4:
>>> Â * merge timer-tegra210.c in previous version into timer-tegra20.c
>>> v3:
>>> Â * use timer-of API
>>> v2:
>>> Â * add error clean-up code
>>> ---
>>> Â drivers/clocksource/KconfigÂÂÂÂÂÂÂÂ |ÂÂ 2 +-
>>> Â drivers/clocksource/timer-tegra20.c | 369 ++++++++++++++++++++--------
>>> Â include/linux/cpuhotplug.hÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>> Â 3 files changed, 272 insertions(+), 100 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index a9e26f6a81a1..6af78534a285 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -131,7 +131,7 @@ config SUN5I_HSTIMER
>>> Â config TEGRA_TIMER
>>> ÂÂÂÂÂ bool "Tegra timer driver" if COMPILE_TEST
>>> ÂÂÂÂÂ select CLKSRC_MMIO
>>> -ÂÂÂ depends on ARM
>>> +ÂÂÂ select TIMER_OF
>>> ÂÂÂÂÂ help
>>> ÂÂÂÂÂÂÂ Enables support for the Tegra driver.
>>> Â diff --git a/drivers/clocksource/timer-tegra20.c
>>> b/drivers/clocksource/timer-tegra20.c
>>> index 4293943f4e2b..96a809341c9b 100644
>>> --- a/drivers/clocksource/timer-tegra20.c
>>> +++ b/drivers/clocksource/timer-tegra20.c
>>> @@ -15,21 +15,24 @@
>>> ÂÂ *
>>> ÂÂ */
>>> Â -#include <linux/init.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/clockchips.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/cpumask.h>
>>> +#include <linux/delay.h>
>>> Â #include <linux/err.h>
>>> -#include <linux/time.h>
>>> Â #include <linux/interrupt.h>
>>> -#include <linux/irq.h>
>>> -#include <linux/clockchips.h>
>>> -#include <linux/clocksource.h>
>>> -#include <linux/clk.h>
>>> -#include <linux/io.h>
>>> Â #include <linux/of_address.h>
>>> Â #include <linux/of_irq.h>
>>> -#include <linux/sched_clock.h>
>>> -#include <linux/delay.h>
>>> +#include <linux/percpu.h>
>>> +#include <linux/syscore_ops.h>
>>> +#include <linux/time.h>
>>> +
>>> +#include "timer-of.h"
>>> Â +#ifdef CONFIG_ARM
>>> Â #include <asm/mach/time.h>
>>> +#endif
>>> Â Â #define RTC_SECONDSÂÂÂÂÂÂÂÂÂÂÂ 0x08
>>> Â #define RTC_SHADOW_SECONDSÂÂÂÂ 0x0c
>>> @@ -43,70 +46,147 @@
>>> Â #define TIMER2_BASE 0x8
>>> Â #define TIMER3_BASE 0x50
>>> Â #define TIMER4_BASE 0x58
>>> -
>>> -#define TIMER_PTV 0x0
>>> -#define TIMER_PCR 0x4
>>> -
>>> +#define TIMER10_BASE 0x90
>>> +
>>> +#define TIMER_PTVÂÂÂÂÂÂÂ 0x0
>>> +#define TIMER_PTV_ENÂÂÂÂÂÂÂ BIT(31)
>>> +#define TIMER_PTV_PERÂÂÂÂÂÂÂ BIT(30)
>>> +#define TIMER_PCRÂÂÂÂÂÂÂ 0x4
>>> +#define TIMER_PCR_INTR_CLRÂÂÂ BIT(30)
>>> +
>>> +#ifdef CONFIG_ARM
>>> +#define TIMER_BASE TIMER3_BASE
>>> +#else
>>> +#define TIMER_BASE TIMER10_BASE
>>> +#endif
>>> +#define TIMER10_IRQ_IDXÂÂÂÂÂÂÂ 10
>>> +#define TIMER_FOR_CPU(cpu) (TIMER_BASE + (cpu) * 8)
>>> +#define IRQ_IDX_FOR_CPU(cpu)ÂÂÂ (TIMER10_IRQ_IDX + cpu)
>>
>> TIMER10_IRQ_IDX and IRQ_IDX_FOR_CPU are only applicable to ARM64 and so
>> we should probably not defined for ARM to avoid any confusion.
> Okay, will do.
>>
>> Furthermore, a lot of these TIMERx_BASE definitions are unused AFAICT.
>> Would be good to get rid of these.
> 
> Okay.
>>
>> Maybe we could just have ...
>>
>> Â +#ifdef CONFIG_ARM
>> Â +#define TIMER_CPU0 3
>> Â +#else
>> Â +#define TIMER_CPU0 10
>> Â +#endif
>> Â +#define TIMER_BASE_FOR_CPU(cpu) ((TIMER_CPU0 + cpu) * 8)
>> Â +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
>>
> This can't get the timer base address. I think you mean ...
> 
> +#ifdef CONFIG_ARM
> +#define TIMER_CPU0 0x50 /* TIMER3 */
> +#else
> +#define TIMER_CPU0 0x90 /* TIMER10 */
> +#endif
> +#define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)

Ah I see.

> This doesn't need.
> +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
How come? Don't you still need to know the timer index for a given CPU?

Cheers
Jon

-- 
nvpublic