| Message ID | 1363210048-3334-1-git-send-email-csd@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/13/2013 02:27 PM, Christian Daudt wrote: > This adds support for the Broadcom timer, used in the following SoCs: > BCM11130, BCM11140, BCM11351, BCM28145, BCM28155 [snip] > Signed-off-by: Christian Daudt <csd@broadcom.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> > Acked-by: John Stultz <john.stultz@linaro.org> > Reviewed-by: Stephen Warren <swarren@nvidia.com> Hey Olof, So Christian mentioned you were hoping I'd pull these in through my tree, but I'd really rather these go via the arch-soc tree, since that is more likely where they will be tested and more intelligently reviewed. I've mentioned before my distaste for the drivers/clocksource directory becoming a dumping ground for arch specific (for the most part arm) clocksource, and more recently clockevent, drivers. I initially wanted the drivers/clocksource directory to be only for cross-arch clocksources, and I fear we'll end up with something like the drivers/rtc directory where its not entirely clear what hardware you actually need for a given driver (might as well be rtc-<sha1>.c ;). That said, I realize I've been overruled on this. :) But I'd really rather the patch flow for arch specific clocksource and clockevent drivers go through the arch tree, since there's a better chance folks will be building and testing against other arch specific changes that might cause problems. There's just no way for me to be able to intelligently review these sorts of changes. Now, if there are changes to the clocksource core code, then I definitely want to be in the path there. Additionally meta-drivers like mmio, I should probably be more involved with, so they can be more properly integrated into the clocksource core. Also for drivers that are actually arch shared (ie: things like hpet/acpi_pm where ia64 and x86 share them). But if its arch specific for hardware I don't have, I'd really prefer the arch maintainer be the upstream path. Thomas: Am I being too obstinate here? If not, should we drop "F: drivers/clocksource" from the MAINTAINERS entry? Christian: Sorry for the confusion on all this! thanks -john
On Wed, 13 Mar 2013, John Stultz wrote: > On 03/13/2013 02:27 PM, Christian Daudt wrote: > > This adds support for the Broadcom timer, used in the following SoCs: > > BCM11130, BCM11140, BCM11351, BCM28145, BCM28155 > [snip] > > Signed-off-by: Christian Daudt <csd@broadcom.com> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > Acked-by: John Stultz <john.stultz@linaro.org> > > Reviewed-by: Stephen Warren <swarren@nvidia.com> > > Hey Olof, > So Christian mentioned you were hoping I'd pull these in through my tree, > but I'd really rather these go via the arch-soc tree, since that is more > likely where they will be tested and more intelligently reviewed. > > I've mentioned before my distaste for the drivers/clocksource directory > becoming a dumping ground for arch specific (for the most part arm) > clocksource, and more recently clockevent, drivers. I initially wanted the OTOH, if they are all in one place we have a better chance to analyze them in bulk, find similarities and patterns for consolidation. And it's simpler for the developer of a new SoC support to find the matching driver for his IP block instead of copying stuff from one mach directory to the next. We've been there :) > But I'd really rather the patch flow for arch specific clocksource and > clockevent drivers go through the arch tree, since there's a better chance > folks will be building and testing against other arch specific changes that > might cause problems. There's just no way for me to be able to intelligently > review these sorts of changes. I agree with the build and test part, but you can still review the code in general - correctness of the particular hw details aside. I just opened a random one there and found: static cycle_t vt8500_timer_read(struct clocksource *cs) { int loops = msecs_to_loops(10); writel(3, regbase + TIMER_CTRL_VAL); while ((readl((regbase + TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE) && --loops) cpu_relax(); return readl(regbase + TIMER_COUNT_VAL); You don't need particular hw knowledge to see that this looks more than fishy at least without comments describing the hardware designers brainfart. > Now, if there are changes to the clocksource core code, then I definitely want > to be in the path there. Additionally meta-drivers like mmio, I should > probably be more involved with, so they can be more properly integrated into > the clocksource core. Also for drivers that are actually arch shared (ie: > things like hpet/acpi_pm where ia64 and x86 share them). > > But if its arch specific for hardware I don't have, I'd really prefer the arch > maintainer be the upstream path. > > Thomas: Am I being too obstinate here? If not, should we drop "F: > drivers/clocksource" from the MAINTAINERS entry? Maybe we should move the ARM specific ones into drivers/clocksource/arm/ ? Thanks, tglx
On Thursday 14 March 2013, Thomas Gleixner wrote: > > But if its arch specific for hardware I don't have, I'd really prefer the arch > > maintainer be the upstream path. > > > > Thomas: Am I being too obstinate here? If not, should we drop "F: > > drivers/clocksource" from the MAINTAINERS entry? The idea of moving drivers out of arch/* into drivers/* is definitely to have someone who understands the subsystem act as the gatekeeper. There should be very little architecture specific knowledge in those drivers, and I think it's more important to ensure that they are following a sensible understanding of how timekeeping is done than that they are following specific architecture maintainer's preferences. > Maybe we should move the ARM specific ones into > drivers/clocksource/arm/ ? About half the IP blocks we use on ARM are also used on at least one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them by which CPU architecture first starts using them or happens to be more popular at the time does not seem too helpful here. Maybe it's better to have a subdirectory for those clock sources that are used on any SoC, or have subdirectories based on the company that created that part, as we do for ethernet drivers. I wouldn't bother with that until there are a couple of dozen different clock source drivers. Arnd
On Thu, Mar 14, 2013 at 9:03 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 14 March 2013, Thomas Gleixner wrote: >> > But if its arch specific for hardware I don't have, I'd really prefer the arch >> > maintainer be the upstream path. >> > >> > Thomas: Am I being too obstinate here? If not, should we drop "F: >> > drivers/clocksource" from the MAINTAINERS entry? > > The idea of moving drivers out of arch/* into drivers/* is definitely to > have someone who understands the subsystem act as the gatekeeper. There > should be very little architecture specific knowledge in those drivers, > and I think it's more important to ensure that they are following a > sensible understanding of how timekeeping is done than that they > are following specific architecture maintainer's preferences. > >> Maybe we should move the ARM specific ones into >> drivers/clocksource/arm/ ? > > About half the IP blocks we use on ARM are also used on at least > one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them > by which CPU architecture first starts using them or happens to be > more popular at the time does not seem too helpful here. > > Maybe it's better to have a subdirectory for those clock sources > that are used on any SoC, or have subdirectories based on the > company that created that part, as we do for ethernet drivers. > I wouldn't bother with that until there are a couple of dozen > different clock source drivers. So it seems the (weak...) consensus is that it should go through the clocksource tree. John, can you please apply the patch ? Thanks, csd
On 03/28/2013 09:03 AM, Christian Daudt wrote: > On Thu, Mar 14, 2013 at 9:03 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thursday 14 March 2013, Thomas Gleixner wrote: >>>> But if its arch specific for hardware I don't have, I'd really prefer the arch >>>> maintainer be the upstream path. >>>> >>>> Thomas: Am I being too obstinate here? If not, should we drop "F: >>>> drivers/clocksource" from the MAINTAINERS entry? >> The idea of moving drivers out of arch/* into drivers/* is definitely to >> have someone who understands the subsystem act as the gatekeeper. There >> should be very little architecture specific knowledge in those drivers, >> and I think it's more important to ensure that they are following a >> sensible understanding of how timekeeping is done than that they >> are following specific architecture maintainer's preferences. Sorry for the slow reply here. Arnd: So I think I better understand the crux of the debate: I'm unfamiliar with the hardware specifics, so I'm pushing the gatekeeping to the SoC maintainers, where as the SoC maintainer (dealing with tons of different SoC's), you're also unfamiliar with the hardware specifics, and would like to push the gatekeeping of the subsystem specific functionality to those maintainers. Given that for each SoC there's tons of subsystem specific functionality that you're having to deal with, I can sympathize with your need to offload some of the gatekeeping. >>> Maybe we should move the ARM specific ones into >>> drivers/clocksource/arm/ ? >> About half the IP blocks we use on ARM are also used on at least >> one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them >> by which CPU architecture first starts using them or happens to be >> more popular at the time does not seem too helpful here. >> >> Maybe it's better to have a subdirectory for those clock sources >> that are used on any SoC, or have subdirectories based on the >> company that created that part, as we do for ethernet drivers. >> I wouldn't bother with that until there are a couple of dozen >> different clock source drivers. So having had a few days to think about this, I think what usually rubs me the wrong way when I get driver/clocksource submissions, is that for 99% of it, they *aren't clocksource drivers*. Most of the code is *clockevent* driver logic, and then maybe 1-5 lines of actual clocksource code. Now, I know the reason for this is often the clocksource and clockevent drivers are backed by the same hardware, and since there's no clockevent directory, might as well have it all in a single file somewhere. But mixing the different subsystem drivers together causes some of the maintenance confusion here. So instead of creating drivers/clocksource/arch/ directories, what I'd propose is we create a drivers/clockevent directory to handle the actual clockevent code. I think this would better delineate the lines of responsibility on the gatekeeper side (that being Thomas or maybe someone else who has an interest in the subtleties of how various hardware timers are be broken-by-design ;), and I'd be much happier taking clocksource code where I felt I had a reasonable chance of noticing bugs. Thomas: Not that you need more to maintain, but does this seem semi-reasonable? Do we need to find someone else to help here? That said, at the end of the day, if I take a bad drivers/clocksource patch, what breaks won't be the timekeeping core, it will be an SoC board. So I'll have to really rely on the original clocksource driver authors to help vet incoming patches. This is where I think having the SoC tree as a central point for SoC patches has and advantage, as its less likely breakage will sneak upstream via a subsystem tree. But understanding the need for review help, I think I'm ok with taking on more clocksource specific review. > So it seems the (weak...) consensus is that it should go through the > clocksource tree. John, can you please apply the patch ? Christian: So thanks again for the ping here. So I think I'd prefer Thomas to be the one to apply these sorts (ie: clockevent) of patches in the future, but this time around I'll do it because its been unfair to make you the monkey-in-the-middle as we've tossed the responsibility around (and my stuff goes through Thomas anyway). thanks -john
On 03/13/2013 02:27 PM, Christian Daudt wrote: > This adds support for the Broadcom timer, used in the following SoCs: > BCM11130, BCM11140, BCM11351, BCM28145, BCM28155 > > Updates from V6: > - Split DT portion into a separate patch > > Updates from V5: > - Rebase to latest arm-soc/for-next > > Updates from V4: > - Switch code to use CLOCKSOURCE_OF_DECLARE > > Updates from V3: > - Migrate to 3.9 timer framework updates > > Updates from V2: > - prepend static fns + fields with kona_ > > Updates from V1: > - Rename bcm_timer.c to bcm_kona_timer.c > - Pull .h into bcm_kona_timer.c > - Make timers static > - Clean up comment block > - Switched to using clockevents_config_and_register > - Added an error to the get_timer loop if it repeats too much > - Added to Documentation/devicetree/bindings/arm/bcm/bcm,kona-timer.txt > - Added missing readl to timer_disable_and_clear > > Note: bcm,kona-timer was kept as the 'compatible' field to make it > specific enough for when there are multiple bcm timers (bcm,timer is > too generic). > > Signed-off-by: Christian Daudt <csd@broadcom.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> > Acked-by: John Stultz <john.stultz@linaro.org> > Reviewed-by: Stephen Warren <swarren@nvidia.com> Applied to my fortglx/3.10/time branch. thanks -john
On Thursday 28 March 2013, John Stultz wrote: > On 03/28/2013 09:03 AM, Christian Daudt wrote: > >>> Maybe we should move the ARM specific ones into > >>> drivers/clocksource/arm/ ? > >> About half the IP blocks we use on ARM are also used on at least > >> one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them > >> by which CPU architecture first starts using them or happens to be > >> more popular at the time does not seem too helpful here. > >> > >> Maybe it's better to have a subdirectory for those clock sources > >> that are used on any SoC, or have subdirectories based on the > >> company that created that part, as we do for ethernet drivers. > >> I wouldn't bother with that until there are a couple of dozen > >> different clock source drivers. > > So having had a few days to think about this, I think what usually rubs > me the wrong way when I get driver/clocksource submissions, is that for > 99% of it, they *aren't clocksource drivers*. Most of the code is > *clockevent* driver logic, and then maybe 1-5 lines of actual > clocksource code. > > Now, I know the reason for this is often the clocksource and clockevent > drivers are backed by the same hardware, and since there's no clockevent > directory, might as well have it all in a single file somewhere. But > mixing the different subsystem drivers together causes some of the > maintenance confusion here. > > So instead of creating drivers/clocksource/arch/ directories, what I'd > propose is we create a drivers/clockevent directory to handle the actual > clockevent code. I think this would better delineate the lines of > responsibility on the gatekeeper side (that being Thomas or maybe > someone else who has an interest in the subtleties of how various > hardware timers are be broken-by-design ;), and I'd be much happier > taking clocksource code where I felt I had a reasonable chance of > noticing bugs. Yes, this sounds like a good idea. > Thomas: Not that you need more to maintain, but does this seem > semi-reasonable? Do we need to find someone else to help here? > > That said, at the end of the day, if I take a bad drivers/clocksource > patch, what breaks won't be the timekeeping core, it will be an SoC > board. So I'll have to really rely on the original clocksource driver > authors to help vet incoming patches. This is where I think having the > SoC tree as a central point for SoC patches has and advantage, as its > less likely breakage will sneak upstream via a subsystem tree. But > understanding the need for review help, I think I'm ok with taking on > more clocksource specific review. I think the important point for you to realize is that you don't need to worry about breaking a specific SoC. The patches will come from someone who is using that hardware in the end. What we need from clock{source,event} subsystem maintainers is perspective of how things fit into the subsystem and to provide review like: - this driver is not using CLOCKSOURCE_OF_DECLARE when it should - that driver only handles clockevent, don't put it into drivers/clocksource - I don't like your coding style, it doesn't fit in with the other drivers in this subsystem. - what you do can be implemented much simpler using the generic infrastructure introduced by that earlier driver. Arnd
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index bf02471..f112895 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -6,6 +6,7 @@ config ARCH_BCM select ARM_ERRATA_764369 if SMP select ARM_GIC select CPU_V7 + select CLKSRC_OF select GENERIC_CLOCKEVENTS select GENERIC_TIME select GPIO_BCM diff --git a/arch/arm/mach-bcm/board_bcm.c b/arch/arm/mach-bcm/board_bcm.c index f0f9aba..2595935 100644 --- a/arch/arm/mach-bcm/board_bcm.c +++ b/arch/arm/mach-bcm/board_bcm.c @@ -16,14 +16,11 @@ #include <linux/device.h> #include <linux/platform_device.h> #include <linux/irqchip.h> +#include <linux/clocksource.h> #include <asm/mach/arch.h> #include <asm/mach/time.h> -static void timer_init(void) -{ -} - static void __init board_init(void) { @@ -35,7 +32,7 @@ static const char * const bcm11351_dt_compat[] = { "bcm,bcm11351", NULL, }; DT_MACHINE_START(BCM11351_DT, "Broadcom Application Processor") .init_irq = irqchip_init, - .init_time = timer_init, + .init_time = clocksource_of_init, .init_machine = board_init, .dt_compat = bcm11351_dt_compat, MACHINE_END diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 4d8283a..96e2531 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_ARCH_BCM2835) += bcm2835_timer.o obj-$(CONFIG_SUNXI_TIMER) += sunxi_timer.o obj-$(CONFIG_ARCH_TEGRA) += tegra20_timer.o obj-$(CONFIG_VT8500_TIMER) += vt8500_timer.o +obj-$(CONFIG_ARCH_BCM) += bcm_kona_timer.o obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c new file mode 100644 index 0000000..350f493 --- /dev/null +++ b/drivers/clocksource/bcm_kona_timer.c @@ -0,0 +1,211 @@ +/* + * 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> + + +#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 + +struct kona_bcm_timers { + int tmr_irq; + void __iomem *tmr_regs; +}; + +static struct kona_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 kona_timer_disable_and_clear(void __iomem *base) +{ + uint32_t reg; + + /* + * clear and disable interrupts + * We are using compare/match register 0 for our system interrupts + */ + reg = readl(base + KONA_GPTIMER_STCS_OFFSET); + + /* 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 +kona_timer_get_counter(void *timer_base, uint32_t *msw, uint32_t *lsw) +{ + void __iomem *base = IOMEM(timer_base); + int loop_limit = 4; + + /* + * 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 (--loop_limit) { + *msw = readl(base + KONA_GPTIMER_STCHI_OFFSET); + *lsw = readl(base + KONA_GPTIMER_STCLO_OFFSET); + if (*msw == readl(base + KONA_GPTIMER_STCHI_OFFSET)) + break; + } + if (!loop_limit) { + pr_err("bcm_kona_timer: getting counter failed.\n"); + pr_err(" Timer will be impacted\n"); + } + + return; +} + +static const struct of_device_id bcm_timer_ids[] __initconst = { + {.compatible = "bcm,kona-timer"}, + {}, +}; + +static void __init kona_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); + + kona_timer_disable_and_clear(timers.tmr_regs); +} + +static int kona_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; + + kona_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 kona_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: + kona_timer_disable_and_clear(timers.tmr_regs); + } +} + +static struct clock_event_device kona_clockevent_timer = { + .name = "timer 1", + .features = CLOCK_EVT_FEAT_ONESHOT, + .set_next_event = kona_timer_set_next_event, + .set_mode = kona_timer_set_mode +}; + +static void __init kona_timer_clockevents_init(void) +{ + kona_clockevent_timer.cpumask = cpumask_of(0); + clockevents_config_and_register(&kona_clockevent_timer, + arch_timer_rate, 6, 0xffffffff); +} + +static irqreturn_t kona_timer_interrupt(int irq, void *dev_id) +{ + struct clock_event_device *evt = &kona_clockevent_timer; + + kona_timer_disable_and_clear(timers.tmr_regs); + evt->event_handler(evt); + return IRQ_HANDLED; +} + +static struct irqaction kona_timer_irq = { + .name = "Kona Timer Tick", + .flags = IRQF_TIMER, + .handler = kona_timer_interrupt, +}; + +static void __init kona_timer_init(void) +{ + kona_timers_init(); + kona_timer_clockevents_init(); + setup_irq(timers.tmr_irq, &kona_timer_irq); + kona_timer_set_next_event((arch_timer_rate / HZ), NULL); +} + +CLOCKSOURCE_OF_DECLARE(bcm_kona, "bcm,kona-timer", + kona_timer_init);