| Message ID | 1430390662-18246-2-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > The header asm/hardware/arm_timer.h is included in various machine > specific files to access TIMER_CTRL and initialise to a known state. > However that's not required as the clock{source,event} driver timer-sp > initialises all the timer being used. I believe the idea is not to initialize the timers being used, but the ones not being used and perhaps left running by the bootloader. Cases where the interrupt is shared could cause a problem. Rob
On 30/04/15 15:09, Rob Herring wrote: > On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> The header asm/hardware/arm_timer.h is included in various machine >> specific files to access TIMER_CTRL and initialise to a known state. >> However that's not required as the clock{source,event} driver timer-sp >> initialises all the timer being used. > > I believe the idea is not to initialize the timers being used, but the > ones not being used and perhaps left running by the bootloader. Cases > where the interrupt is shared could cause a problem. > Ah OK, makes sense. I will wait for Russell to confirm. The main idea was to keep the header file having offsets local to driver/clocksource and avoid sharing it in include/linux but looks like that's not possible. Regards, Sudeep
Hi Russell, On 30/04/15 15:19, Sudeep Holla wrote: > > > On 30/04/15 15:09, Rob Herring wrote: >> On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> The header asm/hardware/arm_timer.h is included in various machine >>> specific files to access TIMER_CTRL and initialise to a known state. >>> However that's not required as the clock{source,event} driver timer-sp >>> initialises all the timer being used. >> >> I believe the idea is not to initialize the timers being used, but the >> ones not being used and perhaps left running by the bootloader. Cases >> where the interrupt is shared could cause a problem. >> Russell, can you confirm if that's the case ? > > Ah OK, makes sense. I will wait for Russell to confirm. The main idea > was to keep the header file having offsets local to driver/clocksource > and avoid sharing it in include/linux but looks like that's not possible. > Since we need this driver on ARM64, we might have to end up sharing the header file with offsets if required for ARM platforms(though it would be good to avoid it if there's any better alternative solution than that) Regards, Sudeep
On Fri, May 15, 2015 at 07:03:34PM +0100, Sudeep Holla wrote: > On 30/04/15 15:19, Sudeep Holla wrote: > >On 30/04/15 15:09, Rob Herring wrote: > >>On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>>The header asm/hardware/arm_timer.h is included in various machine > >>>specific files to access TIMER_CTRL and initialise to a known state. > >>>However that's not required as the clock{source,event} driver timer-sp > >>>initialises all the timer being used. > >> > >>I believe the idea is not to initialize the timers being used, but the > >>ones not being used and perhaps left running by the bootloader. Cases > >>where the interrupt is shared could cause a problem. > > Russell, can you confirm if that's the case ? Unless you want to test all these platforms, I would suggest we assume this is the case. The comments even state "Initialise to a known state (all timers off)". > >Ah OK, makes sense. I will wait for Russell to confirm. The main idea > >was to keep the header file having offsets local to driver/clocksource > >and avoid sharing it in include/linux but looks like that's not possible. > > Since we need this driver on ARM64, we might have to end up sharing the > header file with offsets if required for ARM platforms(though it would > be good to avoid it if there's any better alternative solution than that) Can you not just move the definitions to include/clocksource/timer-sp804.h and add some SP804_ prefix to avoid name collisions?
On 18/05/15 11:33, Catalin Marinas wrote: > On Fri, May 15, 2015 at 07:03:34PM +0100, Sudeep Holla wrote: >> On 30/04/15 15:19, Sudeep Holla wrote: >>> On 30/04/15 15:09, Rob Herring wrote: >>>> On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>>> The header asm/hardware/arm_timer.h is included in various machine >>>>> specific files to access TIMER_CTRL and initialise to a known state. >>>>> However that's not required as the clock{source,event} driver timer-sp >>>>> initialises all the timer being used. >>>> >>>> I believe the idea is not to initialize the timers being used, but the >>>> ones not being used and perhaps left running by the bootloader. Cases >>>> where the interrupt is shared could cause a problem. >> >> Russell, can you confirm if that's the case ? > > Unless you want to test all these platforms, I would suggest we assume > this is the case. The comments even state "Initialise to a known state > (all timers off)". > OK makes sense. >>> Ah OK, makes sense. I will wait for Russell to confirm. The main idea >>> was to keep the header file having offsets local to driver/clocksource >>> and avoid sharing it in include/linux but looks like that's not possible. >> >> Since we need this driver on ARM64, we might have to end up sharing the >> header file with offsets if required for ARM platforms(though it would >> be good to avoid it if there's any better alternative solution than that) > > Can you not just move the definitions to > include/clocksource/timer-sp804.h and add some SP804_ prefix to avoid > name collisions? > Yes I can do that and that's the alternate plan, wanted to avoid having SP804 timer internal register offsets in that header file hoping to contain those in the driver files if possible. Regards, Sudeep
On Thu, Apr 30, 2015 at 09:09:47AM -0500, Rob Herring wrote: > On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > The header asm/hardware/arm_timer.h is included in various machine > > specific files to access TIMER_CTRL and initialise to a known state. > > However that's not required as the clock{source,event} driver timer-sp > > initialises all the timer being used. > > I believe the idea is not to initialize the timers being used, but the > ones not being used and perhaps left running by the bootloader. Cases > where the interrupt is shared could cause a problem. Exactly. IMHO this code needs to stay.
On Thu, Apr 30, 2015 at 03:19:01PM +0100, Sudeep Holla wrote: > > > On 30/04/15 15:09, Rob Herring wrote: > >On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>The header asm/hardware/arm_timer.h is included in various machine > >>specific files to access TIMER_CTRL and initialise to a known state. > >>However that's not required as the clock{source,event} driver timer-sp > >>initialises all the timer being used. > > > >I believe the idea is not to initialize the timers being used, but the > >ones not being used and perhaps left running by the bootloader. Cases > >where the interrupt is shared could cause a problem. > > > > Ah OK, makes sense. I will wait for Russell to confirm. The main idea > was to keep the header file having offsets local to driver/clocksource > and avoid sharing it in include/linux but looks like that's not possible. An alternative would be to have a new function, something like sp804_disable() which takes the virtual address of the timer. That'd still allow the platforms to disable all timers, but without exposing the register stuff to them.
Hi Russell, On 18/05/15 11:40, Russell King - ARM Linux wrote: > On Thu, Apr 30, 2015 at 09:09:47AM -0500, Rob Herring wrote: >> On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> The header asm/hardware/arm_timer.h is included in various machine >>> specific files to access TIMER_CTRL and initialise to a known state. >>> However that's not required as the clock{source,event} driver timer-sp >>> initialises all the timer being used. >> >> I believe the idea is not to initialize the timers being used, but the >> ones not being used and perhaps left running by the bootloader. Cases >> where the interrupt is shared could cause a problem. > > Exactly. IMHO this code needs to stay. > Thanks for confirming. Regards, Sudeep
On 18/05/15 11:42, Russell King - ARM Linux wrote: > On Thu, Apr 30, 2015 at 03:19:01PM +0100, Sudeep Holla wrote: >> >> >> On 30/04/15 15:09, Rob Herring wrote: >>> On Thu, Apr 30, 2015 at 5:44 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> The header asm/hardware/arm_timer.h is included in various machine >>>> specific files to access TIMER_CTRL and initialise to a known state. >>>> However that's not required as the clock{source,event} driver timer-sp >>>> initialises all the timer being used. >>> >>> I believe the idea is not to initialize the timers being used, but the >>> ones not being used and perhaps left running by the bootloader. Cases >>> where the interrupt is shared could cause a problem. >>> >> >> Ah OK, makes sense. I will wait for Russell to confirm. The main idea >> was to keep the header file having offsets local to driver/clocksource >> and avoid sharing it in include/linux but looks like that's not possible. > > An alternative would be to have a new function, something like > sp804_disable() which takes the virtual address of the timer. > That'd still allow the platforms to disable all timers, but > without exposing the register stuff to them. > Yes that's much better, will re-spin the patch accordingly. Regards, Sudeep
diff --git a/arch/arm/mach-integrator/integrator_ap.c b/arch/arm/mach-integrator/integrator_ap.c index 30003ba447a5..5b0e363fe5ba 100644 --- a/arch/arm/mach-integrator/integrator_ap.c +++ b/arch/arm/mach-integrator/integrator_ap.c @@ -37,7 +37,6 @@ #include <linux/stat.h> #include <linux/termios.h> -#include <asm/hardware/arm_timer.h> #include <asm/setup.h> #include <asm/param.h> /* HZ */ #include <asm/mach-types.h> diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c index c309593abdb2..dd9c12d995db 100644 --- a/arch/arm/mach-realview/core.c +++ b/arch/arm/mach-realview/core.c @@ -38,7 +38,6 @@ #include <mach/hardware.h> #include <asm/irq.h> #include <asm/mach-types.h> -#include <asm/hardware/arm_timer.h> #include <asm/hardware/icst.h> #include <asm/mach/arch.h> @@ -378,14 +377,6 @@ void __init realview_timer_init(unsigned int timer_irq) (REALVIEW_TIMCLK << REALVIEW_TIMER4_EnSel) | val, __io_address(REALVIEW_SCTL_BASE)); - /* - * Initialise to a known state (all timers off) - */ - writel(0, timer0_va_base + TIMER_CTRL); - writel(0, timer1_va_base + TIMER_CTRL); - writel(0, timer2_va_base + TIMER_CTRL); - writel(0, timer3_va_base + TIMER_CTRL); - sp804_clocksource_init(timer3_va_base, "timer3"); sp804_clockevents_init(timer0_va_base, timer_irq, "timer0"); } diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c index 6ea09fe53426..a5a57e453698 100644 --- a/arch/arm/mach-versatile/core.c +++ b/arch/arm/mach-versatile/core.c @@ -42,7 +42,6 @@ #include <linux/reboot.h> #include <asm/irq.h> -#include <asm/hardware/arm_timer.h> #include <asm/hardware/icst.h> #include <asm/mach-types.h> @@ -794,15 +793,6 @@ void __init versatile_init(void) */ void __init versatile_timer_init(void) { - - /* - * Initialise to a known state (all timers off) - */ - writel(0, TIMER0_VA_BASE + TIMER_CTRL); - writel(0, TIMER1_VA_BASE + TIMER_CTRL); - writel(0, TIMER2_VA_BASE + TIMER_CTRL); - writel(0, TIMER3_VA_BASE + TIMER_CTRL); - sp804_clocksource_init(TIMER3_VA_BASE, "timer3"); sp804_clockevents_init(TIMER0_VA_BASE, IRQ_TIMERINT0_1, "timer0"); }
The header asm/hardware/arm_timer.h is included in various machine specific files to access TIMER_CTRL and initialise to a known state. However that's not required as the clock{source,event} driver timer-sp initialises all the timer being used. This patch removes the redundant code in timer_init and also the inclusion of asm/hardware/arm_timer.h header. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Cc: Russell King <linux@arm.linux.org.uk> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Olof Johansson <olof@lixom.net> --- arch/arm/mach-integrator/integrator_ap.c | 1 - arch/arm/mach-realview/core.c | 9 --------- arch/arm/mach-versatile/core.c | 10 ---------- 3 files changed, 20 deletions(-) -- 1.9.1