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
