| Message ID | 1354593324-21300-1-git-send-email-csd@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 04 December 2012, Christian Daudt wrote: > 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 > > Signed-off-by: Christian Daudt <csd@broadcom.com> Acked-by: Arnd Bergmann <arnd@arndb.de> > > -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
On 12/03/2012 08:55 PM, Christian Daudt wrote: > 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 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. > diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi > + timer@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. > diff --git a/drivers/clocksource/bcm_timer.c b/drivers/clocksource/bcm_timer.c 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. > +struct bcm_timers { > + int tmr_irq; > + void __iomem *tmr_regs; > +}; > + > +struct bcm_timers timers; Should that be static? > +/* 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: > +/* > + * We use the peripheral timers for system tick, the cpu global timer for > + * profile tick > + */ > +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? > +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? > +static void __init timer_clockevents_init(void) Can this function use clockevents_config_and_register() to avoid a bunch of the manual calculations and assignments? > diff --git a/drivers/clocksource/bcm_timer.h b/drivers/clocksource/bcm_timer.h You may as well move these definitions into the .c file since they don't need to be public?
On 12-12-04 06:54 PM, Stephen Warren wrote: > On 12/03/2012 08:55 PM, Christian Daudt wrote: >> 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 > 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. 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. > >> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi >> + timer@35006000 { >> + compatible = "bcm,kona-timer"; > Is there DT binding documentation for this? I'll add one > 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" >> diff --git a/drivers/clocksource/bcm_timer.c b/drivers/clocksource/bcm_timer.c > 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. 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... >> +struct bcm_timers { >> + int tmr_irq; >> + void __iomem *tmr_regs; >> +}; >> + >> +struct bcm_timers timers; > Should that be static? changed. > >> +/* 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: > >> +/* >> + * We use the peripheral timers for system tick, the cpu global timer for >> + * profile tick >> + */ changed. >> +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. >> +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. >> +static void __init timer_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. > >> diff --git a/drivers/clocksource/bcm_timer.h b/drivers/clocksource/bcm_timer.h > You may as well move these definitions into the .c file since they don't > need to be public? agreed. Thanks, csd
On Wednesday 05 December 2012, Christian Daudt wrote: > On 12-12-04 06:54 PM, Stephen Warren wrote: > > 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. > >> diff --git a/drivers/clocksource/bcm_timer.c b/drivers/clocksource/bcm_timer.c > > 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. > > 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
diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi index ad13588..8f71f40 100644 --- a/arch/arm/boot/dts/bcm11351.dtsi +++ b/arch/arm/boot/dts/bcm11351.dtsi @@ -47,4 +47,12 @@ cache-unified; cache-level = <2>; }; + + timer@35006000 { + compatible = "bcm,kona-timer"; + reg = <0x35006000 0x1000>; + interrupts = <0x0 7 0x4>; + clock-frequency = <32768>; + }; + }; diff --git a/arch/arm/mach-bcm/board_bcm.c b/arch/arm/mach-bcm/board_bcm.c index 3a62f1b..2457010 100644 --- a/arch/arm/mach-bcm/board_bcm.c +++ b/arch/arm/mach-bcm/board_bcm.c @@ -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) diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 603be36..fb4fc51 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -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 diff --git a/drivers/clocksource/bcm_timer.c b/drivers/clocksource/bcm_timer.c new file mode 100644 index 0000000..85d1904 --- /dev/null +++ b/drivers/clocksource/bcm_timer.c @@ -0,0 +1,202 @@ +/* + * 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. + */ + +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/interrupt.h> +#include <linux/jiffies.h> +#include <linux/clockchips.h> +#include <linux/types.h> + +#include <linux/io.h> +#include <asm/mach/time.h> + +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> + +#include "bcm_timer.h" + +struct bcm_timers { + int tmr_irq; + void __iomem *tmr_regs; +}; + +struct bcm_timers timers; + +static u32 arch_timer_rate; + +/* We use the peripheral timers for system tick, the cpu global timer for + * profile tick + */ +static void timer_disable_and_clear(void __iomem *base) +{ + uint32_t reg; + + /* clear and disable timer interrupts + * We are using compare/match register 0 for + * our system interrupts + */ + reg = 0; + + /* Clear compare (0) interrupt */ + reg |= 1 << KONA_GPTIMER_STCS_TIMER_MATCH_SHIFT; + /* disable compare */ + reg &= ~(1 << KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT); + + writel(reg, base + KONA_GPTIMER_STCS_OFFSET); + +} + +static void +timer_get_counter(void *timer_base, uint32_t *msw, uint32_t *lsw) +{ + void __iomem *base = IOMEM(timer_base); + + /* Read 64-bit free running counter + * 1. Read hi-word + * 2. Read low-word + * 3. Read hi-word again + * 4.1 + * if new hi-word is not equal to previously read hi-word, then + * start from #1 + * 4.2 + * if new hi-word is equal to previously read hi-word then stop. + */ + + 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; + } + + return; +} + +static const struct of_device_id bcm_timer_ids[] __initconst = { + {.compatible = "bcm,kona-timer"}, + {}, +}; + +static void __init timers_init(void) +{ + struct device_node *node; + u32 freq; + + node = of_find_matching_node(NULL, bcm_timer_ids); + + if (!node) + panic("No timer"); + + if (!of_property_read_u32(node, "clock-frequency", &freq)) + arch_timer_rate = freq; + else + panic("clock-frequency not set in the .dts file"); + + /* Setup IRQ numbers */ + timers.tmr_irq = irq_of_parse_and_map(node, 0); + + /* Setup IO addresses */ + timers.tmr_regs = of_iomap(node, 0); + + timer_disable_and_clear(timers.tmr_regs); +} + +static int timer_set_next_event(unsigned long clc, + struct clock_event_device *unused) +{ + /* timer (0) is disabled by the timer interrupt already + * so, here we reload the next event value and re-enable + * the timer + * + * This way, we are potentially losing the time between + * timer-interrupt->set_next_event. CPU local timers, when + * they come in should get rid of skew + */ + + uint32_t lsw, msw; + uint32_t reg; + + timer_get_counter(timers.tmr_regs, &msw, &lsw); + + /* Load the "next" event tick value */ + writel(lsw + clc, timers.tmr_regs + KONA_GPTIMER_STCM0_OFFSET); + + /* Enable compare */ + reg = readl(timers.tmr_regs + KONA_GPTIMER_STCS_OFFSET); + reg |= (1 << KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT); + writel(reg, timers.tmr_regs + KONA_GPTIMER_STCS_OFFSET); + + return 0; +} + +static void timer_set_mode(enum clock_event_mode mode, + struct clock_event_device *unused) +{ + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + /* by default mode is one shot don't do any thing */ + break; + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + default: + timer_disable_and_clear(timers.tmr_regs); + } +} + +static struct clock_event_device clockevent_timer = { + .name = "timer 1", + .features = CLOCK_EVT_FEAT_ONESHOT, + .shift = 32, + .set_next_event = timer_set_next_event, + .set_mode = timer_set_mode +}; + +static void __init timer_clockevents_init(void) +{ + clockevent_timer.mult = div_sc(arch_timer_rate, NSEC_PER_SEC, + clockevent_timer.shift); + + clockevent_timer.max_delta_ns = + clockevent_delta2ns(0xffffffff, &clockevent_timer); + + clockevent_timer.min_delta_ns = + clockevent_delta2ns(6, &clockevent_timer); + + clockevent_timer.cpumask = cpumask_of(0); + clockevents_register_device(&clockevent_timer); +} + +static irqreturn_t timer_interrupt(int irq, void *dev_id) +{ + struct clock_event_device *evt = &clockevent_timer; + + timer_disable_and_clear(timers.tmr_regs); + evt->event_handler(evt); + return IRQ_HANDLED; +} + +static struct irqaction timer_irq = { + .name = "Kona Timer Tick", + .flags = IRQF_TIMER, + .handler = timer_interrupt, +}; + +void __init bcm_timer_init(void) +{ + timers_init(); + timer_clockevents_init(); + setup_irq(timers.tmr_irq, &timer_irq); + timer_set_next_event((arch_timer_rate / HZ), NULL); +} diff --git a/drivers/clocksource/bcm_timer.h b/drivers/clocksource/bcm_timer.h new file mode 100644 index 0000000..96a6280 --- /dev/null +++ b/drivers/clocksource/bcm_timer.h @@ -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__ */
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 Signed-off-by: Christian Daudt <csd@broadcom.com>