Messages in this thread | | | Subject | Re: [PATCH V5 2/7] clocksource: tegra: add Tegra210 timer support | From | Joseph Lo <> | Date | Fri, 1 Feb 2019 22:39:46 +0800 |
| |
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@linaro.org> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Joseph Lo <josephl@nvidia.com> >> Acked-by: Thierry Reding <treding@nvidia.com> >> --- >> 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)
This doesn't need. +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
Will fix above accordingly and adding your ack tag.
Thanks, Joseph
> Otherwise looks good to me. > > Cheers > Jon >
| |