ARM: bcm281xx: Add timer driver
Subsystems (6)
PATCH 1/1 ARM: bcm281xx: Add timer driver
This adds support for the Broadcom timer, used in the following SoCs:
BCM11130, BCM11140, BCM11351, BCM28145, BCM28155
This patch needs the arm-soc/soc/next branch
arch/arm/boot/dts/bcm11351.dtsi +8 -0
@@ -47,4 +47,12 @@ cache-unified; cache-level = <2>; }; + + timer at 35006000 { + compatible = "bcm,kona-timer"; + reg = <0x35006000 0x1000>; + interrupts = <0x0 7 0x4>; + clock-frequency = <32768>; + }; + };
arch/arm/mach-bcm/board_bcm.c +2 -4
@@ -27,12 +27,10 @@ static const struct of_device_id irq_match[] = { {} }; -static void timer_init (void ) -{ -} +extern void bcm_timer_init (void );static struct sys_timer timer = { - .init = timer_init, + .init = bcm_timer_init, };static void __init init_irq (void )
drivers/clocksource/Makefile +1 -0
@@ -14,5 +14,6 @@ obj-$(CONFIG_DW_APB_TIMER_OF) += dw_apb_timer_of.o obj-$(CONFIG_CLKSRC_DBX500_PRCMU) += clksrc-dbx500-prcmu.o obj-$(CONFIG_ARMADA_370_XP_TIMER) += time-armada-370-xp.o obj-$(CONFIG_ARCH_BCM2835) += bcm2835_timer.o +obj-$(CONFIG_ARCH_BCM) += bcm_timer.o obj-$(CONFIG_CLKSRC_ARM_GENERIC) += arm_generic.o
drivers/clocksource/bcm_timer.c new file +202 -0
202 lines not on this page · raw message
drivers/clocksource/bcm_timer.h new file +25 -0
@@ -0,0 +1,25 @@ +/* +* Copyright (C) 2012 Broadcom Corporation +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation version 2. +* +* This program is distributed "as is" WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __KONA_TIMER_H__ +#define __KONA_TIMER_H__ + +#define KONA_GPTIMER_STCS_OFFSET 0x00000000 +#define KONA_GPTIMER_STCLO_OFFSET 0x00000004 +#define KONA_GPTIMER_STCHI_OFFSET 0x00000008 +#define KONA_GPTIMER_STCM0_OFFSET 0x0000000C + +#define KONA_GPTIMER_STCS_TIMER_MATCH_SHIFT 0 +#define KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT 4 + +#endif /* __KONA_TIMER_H__ */
Signature · 1 line
1.7.1
PATCH 1/1 ARM: bcm281xx: Add timer driver
Quoted 6 lines · Christian Daudt
This adds support for the Broadcom timer, used in the following SoCs:
BCM11130, BCM11140, BCM11351, BCM28145, BCM28155This patch needs the arm-soc/soc/next branch
Signed-off-by Christian Daudt <[email protected]>
arch/arm/mach-bcm/board_bcm.c static const struct of_device_id irq_match[] = { · line 34
Quoted 11 lines
arch/arm/mach-bcm/board_bcm.c +2
-4-static void timer_init (void ) -{ -} +extern void bcm_timer_init (void );static struct sys_timer timer = { - .init = timer_init, + .init = bcm_timer_init, };
This part will confict with the recent patches that kill off the struct sys_timer, but it should be easy to fix.
Arnd
PATCH 1/1 ARM: bcm281xx: Add timer driver
In reply to Christian DaudtQuoted 4 lines · Christian Daudt
This adds support for the Broadcom timer, used in the following SoCs:
BCM11130, BCM11140, BCM11351, BCM28145, BCM28155This patch needs the arm-soc/soc/next branch
I assume this is being applied for 3.9, so it'll want to be rebased on the struct sys_timer removal, which I hope to get into linux-next very early after 3.8-rc1.
arch/arm/boot/dts/bcm11351.dtsi line 52
Quoted 4 lines
arch/arm/boot/dts/bcm11351.dtsi +2
-0+ timer at 35006000 { + compatible = "bcm,kona-timer";
Is there DT binding documentation for this?
I'm slightly worried about "kona". Is it a well-known name outside Broadcom for this HW block? If it really is the name though, it's fine I guess, since it's within the "bcm," name-space here.
drivers/clocksource/bcm_timer.c
Quoted 1 line
drivers/clocksource/bcm_timer.c +0
-0
Is this timer HW used in every Broadcom chip? I wonder if the file shouldn't be named bcm_kona_timer.c to allow co-existence with any others.
drivers/clocksource/bcm_timer.c line 35
Quoted 6 lines
drivers/clocksource/bcm_timer.c +6
-0+struct bcm_timers { +int tmr_irq; +void __iomem *tmr_regs; +}; + +struct bcm_timers timers;
Should that be static?
Quoted 3 lines
+/* We use the peripheral timers for system tick, the cpu global timer for
+ * profile tick
+ */
I think it's usual to leave the /* line empty, so that would be:
Quoted 4 lines
drivers/clocksource/bcm_timer.c +4
-0+/* +* We use the peripheral timers for system tick, the cpu global timer for +* profile tick +*/
drivers/clocksource/bcm_timer.c line 55
Quoted 10 lines
drivers/clocksource/bcm_timer.c +7
-0+static void timer_disable_and_clear (void __iomem *base) + reg =0 ; + +/* Clear compare (0) interrupt */ + reg |=1 <<KONA_GPTIMER_STCS_TIMER_MATCH_SHIFT ; +/* disable compare */ + reg &= ~(1 <<KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT );
That bit isn't set anywhere; is there a need to clear it; should "reg = 0" above be a readl() of the register?
drivers/clocksource/bcm_timer.c line 82
Quoted 9 lines
drivers/clocksource/bcm_timer.c +8
-0+static void +timer_get_counter (void *timer_base,uint32_t *msw,uint32_t *lsw) + while (1) { + *msw =readl (base +KONA_GPTIMER_STCHI_OFFSET ); + *lsw =readl (base +KONA_GPTIMER_STCLO_OFFSET ); +if (*msw ==readl (base +KONA_GPTIMER_STCHI_OFFSET )) +break ; + }
I guess it's very unlikely, but should there be a limit to the loop count here, or a diagnostic if it runs too long?
Quoted 1 line
+static void
__inittimer_clockevents_init(void)
Can this function use clockevents_config_and_register() to avoid a bunch of the manual calculations and assignments?
drivers/clocksource/bcm_timer.h
Quoted 1 line
drivers/clocksource/bcm_timer.h +0
-0
You may as well move these definitions into the .c file since they don't need to be public?
PATCH 1/1 ARM: bcm281xx: Add timer driver
Quoted 8 lines · Stephen Warren
Quoted 4 lines · Christian Daudt
This adds support for the Broadcom timer, used in the following SoCs:
BCM11130, BCM11140, BCM11351, BCM28145, BCM28155This patch needs the arm-soc/soc/next branch
I assume this is being applied for 3.9, so it'll want to be rebased on the struct
sys_timerremoval, which I hope to get into linux-next very early after 3.8-rc1.
Not sure at this point - from my point of view earlier is always better so if it makes it to 3.8 that's fine by me. But ultimately whatever makes sense is fine by me so if 3.8 is not appropriate then 3.9 it is.
Quoted 5 lines
arch/arm/boot/dts/bcm11351.dtsi line 52
Quoted 4 lines
arch/arm/boot/dts/bcm11351.dtsi +2
-0+ timer at 35006000 { + compatible = "bcm,kona-timer";Is there DT binding documentation for this?
I'll add one
Quoted 3 lines
I'm slightly worried about "kona". Is it a well-known name outside Broadcom for this HW block? If it really is the name though, it's fine I guess, since it's within the "bcm," name-space here.
Some of these konas slip by :) This is an internal name, but I don't need to use it here. I'll change this to "bcm,bcm-timer"
Quoted 3 lines
drivers/clocksource/bcm_timer.c
Quoted 1 line
drivers/clocksource/bcm_timer.c +0
-0Is this timer HW used in every Broadcom chip? I wonder if the file shouldn't be named
bcm_kona_timer.cto allow co-existence with any others.
I'm sure it is not used in every Broadcom chip, but it is used in the ones I'm upstreaming at this point. I can always rename it if it turns out that this is no longer the only one, can't I ? I have been struggling a bit with when to use just "bcm" for name, and when to use something else. Internally we've used kona (and a number of other internal only names) but I've been trying to scrub these out of the code going to upstream, as the internal names are meaningless. But then I end up with no name in some cases, and I don't know that that is more helpful than the meaningless name...
Quoted 7 lines
drivers/clocksource/bcm_timer.c line 35
Quoted 6 lines
drivers/clocksource/bcm_timer.c +6
-0+struct bcm_timers { +int tmr_irq; +void __iomem *tmr_regs; +}; + +struct bcm_timers timers;Should that be static?
changed.
Quoted 10 lines
Quoted 4 lines
+/* We use the peripheral timers for system tick, the cpu global timer for
+ * profile tick
+ */I think it's usual to leave the /* line empty, so that would be:
Quoted 4 lines
+/*
+ * We use the peripheral timers for system tick, the cpu global timer for
+ * profile tick
+ */
changed.
Quoted 9 lines
drivers/clocksource/bcm_timer.c line 55
Quoted 7 lines
drivers/clocksource/bcm_timer.c +7
-0+static void timer_disable_and_clear (void __iomem *base) + reg =0 ; + +/* Clear compare (0) interrupt */ + reg |=1 <<KONA_GPTIMER_STCS_TIMER_MATCH_SHIFT ; +/* disable compare */ + reg &= ~(1 <<KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT );That bit isn't set anywhere; is there a need to clear it; should "reg = 0" above be a
readl()of the register?
yes, that is missing. I'll add it.
Quoted 10 lines
drivers/clocksource/bcm_timer.c line 82
Quoted 8 lines
drivers/clocksource/bcm_timer.c +8
-0+static void +timer_get_counter (void *timer_base,uint32_t *msw,uint32_t *lsw) + while (1) { + *msw =readl (base +KONA_GPTIMER_STCHI_OFFSET ); + *lsw =readl (base +KONA_GPTIMER_STCLO_OFFSET ); +if (*msw ==readl (base +KONA_GPTIMER_STCHI_OFFSET )) +break ; + }I guess it's very unlikely, but should there be a limit to the loop count here, or a diagnostic if it runs too long?
I'll add a pr_err if it runs too many times. That should never happen.
Quoted 3 lines
Quoted 1 line
+static void
__inittimer_clockevents_init(void)Can this function use
clockevents_config_and_register()to avoid a bunch of the manual calculations and assignments?
I'll take a look whether it can or not.
Quoted 4 lines
drivers/clocksource/bcm_timer.h
Quoted 2 lines
drivers/clocksource/bcm_timer.h +0
-0You may as well move these definitions into the .c file since they don't need to be public?
agreed.
Thanks,
csd
PATCH 1/1 ARM: bcm281xx: Add timer driver
Quoted 7 lines · Christian Daudt
Quoted 4 lines · Stephen Warren
On 12/03/2012 08:55 PM, Christian Daudt wrote:
I'm slightly worried about "kona". Is it a well-known name outside Broadcom for this HW block? If it really is the name though, it's fine I guess, since it's within the "bcm," name-space here.Some of these konas slip by :) This is an internal name, but I don't need to use it here. I'll change this to "bcm,bcm-timer"
bcm-timer sounds a bit too generic, unless it's the only one used in Broadcom, I don't mind a code name like "kona" if that serves to uniquely identify this timer implementation.
If you don't want to to use that, you should pick a specific mode number and encode that into the compatible string, such as "bcm,bcm28154-timer". You can then use the same string for every SoC whose timer is the same as the one in the bcm28154.
Quoted 4 lines
Quoted 3 lines
drivers/clocksource/bcm_timer.c
Quoted 1 line
drivers/clocksource/bcm_timer.c +0
-0Is this timer HW used in every Broadcom chip? I wonder if the file shouldn't be named
bcm_kona_timer.cto allow co-existence with any others.
Quoted 9 lines
I'm sure it is not used in every Broadcom chip, but it is used in the ones I'm upstreaming at this point. I can always rename it if it turns out that this is no longer the only one, can't I ? I have been struggling a bit with when to use just "bcm" for name, and when to use something else. Internally we've used kona (and a number of other internal only names) but I've been trying to scrub these out of the code going to upstream, as the internal names are meaningless. But then I end up with no name in some cases, and I don't know that that is more helpful than the meaningless name...
If the name means something to you and is not likely to be used elsewhere in broadcom for something different, then it's a good enough name. Leaving out the name is not enough.
Arnd