Create <linux/atmel_tc.h> based on <asm-arm/arch-at91/at91-tc.h> and the
at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
or two of these TC blocks, which include three 16-bit timers that can be
interconnected in various ways.
These TC blocks can be used for external interfacing (such as PWM and
measurement), or used as somewhat quirky sixteen-bit timers.
Signed-off-by: David Brownell <[email protected]>
---
Note that this won't be usable until the AT91 and AT32 platforms
incorporate patches to configure the relevant platform devices.
Those changes are probably 2.6.26 material.
drivers/misc/Kconfig | 8 +
drivers/misc/Makefile | 1
drivers/misc/atmel_tclib.c | 107 +++++++++++++++++++++
include/linux/atmel_tc.h | 221 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 337 insertions(+)
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -22,6 +22,14 @@ config ATMEL_PWM
purposes including software controlled power-efficent backlights
on LCD displays, motor control, and waveform generation.
+config ATMEL_TCLIB
+ bool "Atmel AT32/AT91 Timer/Counter Library"
+ depends on (AVR32 || ARCH_AT91)
+ help
+ Select this if you want a library to allocate the Timer/Counter
+ blocks found on many Atmel processors. This facilitates using
+ these blocks by different drivers despite processor differences.
+
config IBM_ASM
tristate "Device driver for IBM RSA service processor"
depends on X86 && PCI && INPUT && EXPERIMENTAL
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ACER_WMI) += acer-wmi.o
obj-$(CONFIG_ASUS_LAPTOP) += asus-laptop.o
obj-$(CONFIG_ATMEL_PWM) += atmel_pwm.o
obj-$(CONFIG_ATMEL_SSC) += atmel-ssc.o
+obj-$(CONFIG_ATMEL_TCLIB) += atmel_tclib.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
obj-$(CONFIG_LKDTM) += lkdtm.o
obj-$(CONFIG_TIFM_CORE) += tifm_core.o
--- /dev/null
+++ b/drivers/misc/atmel_tclib.c
@@ -0,0 +1,107 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/atmel_tc.h>
+
+
+/*
+ * This is a thin library to solve the problem of how to portably allocate
+ * one of the TC blocks. For simplicity, it doesn't currently expect to
+ * share individual timers between different drivers.
+ */
+
+#if defined(CONFIG_AVR32)
+/* AVR32 has these divide PBB */
+const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#elif defined(CONFIG_ARCH_AT91)
+/* AT91 has these divide MCK */
+const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#endif
+
+/* we "know" that there will be either one or two TC blocks */
+static struct platform_device *blocks[2];
+
+
+/**
+ * atmel_tc_alloc - allocate a specified TC block
+ * @block: which block to allocate
+ * @iomem: used to return its IO memory resource
+ *
+ * Caller allocates a block. If it is available, its I/O space is requested
+ * and returned through the iomem pointer, and the device node for the block
+ * is returned. When it is not available, NULL is returned.
+ *
+ * On some platforms, each TC channel has its own clocks and IRQs. Drivers
+ * should clk_get() and clk_enable() "t0_clk", "t1_clk, and "t2_clk".
+ * In the same vein, they should platform_get_irq() for irqs 0, 1, and 2.
+ * On other platforms, only irq 0 and "t0_clk" will be available; drivers
+ * should handle with both configurations.
+ */
+struct platform_device *atmel_tc_alloc(unsigned block, struct resource **iomem)
+{
+ struct platform_device *tc;
+ struct resource *r;
+
+ if (block >= ARRAY_SIZE(blocks) || !iomem)
+ return NULL;
+
+ tc = blocks[block];
+ if (tc) {
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ if (r)
+ r = request_mem_region(r->start, 256, NULL);
+ *iomem = r;
+ if (!r)
+ tc = NULL;
+ }
+
+ return tc;
+}
+EXPORT_SYMBOL_GPL(atmel_tc_alloc);
+
+/**
+ * atmel_tc_free - release a specified TC block
+ * @block: which block to release
+ *
+ * This reverses the effect of atmel_tc_alloc(), invalidating the resource
+ * returned by that routine and making the TC available to other drivers.
+ */
+void atmel_tc_free(struct platform_device *tc)
+{
+ if (tc->id >= 0 && tc->id < ARRAY_SIZE(blocks)) {
+ struct resource *r;
+
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ release_mem_region(r->start, 256);
+ }
+}
+EXPORT_SYMBOL_GPL(atmel_tc_free);
+
+static int __init tc_probe(struct platform_device *pdev)
+{
+ static char __initdata e2big[] =
+ KERN_ERR "tclib: can't record TC block %d\n";
+
+ if (pdev->id < 0 || pdev->id >= ARRAY_SIZE(blocks)) {
+ printk(e2big, pdev->id);
+ return -ENFILE;
+ }
+ blocks[pdev->id] = pdev;
+ return 0;
+}
+
+static struct platform_driver tc_driver = {
+ .driver.name = "atmel_tcb",
+};
+
+static int __init tc_init(void)
+{
+ return platform_driver_probe(&tc_driver, tc_probe);
+}
+arch_initcall(tc_init);
--- /dev/null
+++ b/include/linux/atmel_tc.h
@@ -0,0 +1,221 @@
+/*
+ * Timer/Counter Unit (TC) registers.
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef ATMEL_TC_H
+#define ATMEL_TC_H
+
+/*
+ * Many 32-bit Atmel SOCs include one or more TC blocks, each of which holds
+ * three general-purpose 16-bit timers. These timers share one register bank.
+ * Depending on the SOC, each timer may have its own clock and IRQ, or those
+ * may be shared by the whole TC block.
+ *
+ * These TC blocks may have up to nine external pins: TCLK0..2 signals for
+ * clocks or clock gates, and per-timer TIOA and TIOB signals used for PWM
+ * or triggering. Those pins need to be set up for use with the TC block,
+ * else they will be used as GPIOs or for a different controller.
+ *
+ * Although we expect each TC block to have a platform_device node, those
+ * nodes are not what drivers bind to. Instead, they ask for a specific
+ * TC block, by number ... which is a common approach on systems with many
+ * timers. Then they use clk_get() and platform_get_irq() to get clock and
+ * IRQ resources.
+ */
+extern struct platform_device *atmel_tc_alloc(unsigned block,
+ struct resource **iomem);
+extern void atmel_tc_free(struct platform_device *tc);
+
+/* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */
+extern const u8 atmel_tc_divisors[5];
+
+
+/*
+ * Two registers have block-wide controls. These are: configuring the three
+ * "external" clocks (or event sources) used by the timer channels; and
+ * synchronizing the timers by resetting them all at once.
+ *
+ * "External" can mean "external to chip" using the TCLK0, TCLK1, or TCLK2
+ * signals. Or, it can mean "external to timer", using the TIOA output from
+ * one of the other two timers that's being run in waveform mode.
+ */
+
+#define ATMEL_TC_BCR 0xc0 /* TC Block Control Register */
+#define ATMEL_TC_SYNC (1 << 0) /* synchronize timers */
+
+#define ATMEL_TC_BMR 0xc4 /* TC Block Mode Register */
+#define ATMEL_TC_TC0XC0S (3 << 0) /* external clock 0 source */
+#define ATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
+#define ATMEL_TC_TC0XC0S_NONE (1 << 0)
+#define ATMEL_TC_TC0XC0S_TIOA1 (2 << 0)
+#define ATMEL_TC_TC0XC0S_TIOA2 (3 << 0)
+#define ATMEL_TC_TC1XC1S (3 << 2) /* external clock 1 source */
+#define ATMEL_TC_TC1XC1S_TCLK1 (0 << 2)
+#define ATMEL_TC_TC1XC1S_NONE (1 << 2)
+#define ATMEL_TC_TC1XC1S_TIOA0 (2 << 2)
+#define ATMEL_TC_TC1XC1S_TIOA2 (3 << 2)
+#define ATMEL_TC_TC2XC2S (3 << 4) /* external clock 2 source */
+#define ATMEL_TC_TC2XC2S_TCLK2 (0 << 4)
+#define ATMEL_TC_TC2XC2S_NONE (1 << 4)
+#define ATMEL_TC_TC2XC2S_TIOA0 (2 << 4)
+#define ATMEL_TC_TC2XC2S_TIOA1 (3 << 4)
+
+
+/*
+ * Each TC block has three "channels", each with one counter and controls.
+ *
+ * Note that the semantics of ATMEL_TC_TIMER_CLOCKx (input clock selection
+ * when it's not "external") is silicon-specific. AT91 platforms use one
+ * set of definitions; AVR32 platforms use a different set. Don't hard-wire
+ * such knowledge into your code, use the global "atmel_tc_divisors" ...
+ * where index N is the divisor for clock N+1, else zero to indicate it uses
+ * the 32 KiHz clock.
+ *
+ * The timers can be chained in various ways, and operated in "waveform"
+ * generation mode (including PWM) or "capture" mode (to time events). In
+ * both modes, behavior can be configured in many ways.
+ *
+ * Each timer has two I/O pins, TIOA and TIOB. Waveform mode uses TIOA as a
+ * PWM output, and TIOB as either another PWM or as a trigger. Capture mode
+ * uses them only as inputs.
+ */
+#define ATMEL_TC_CHAN(idx) ((idx)*0x40)
+#define ATMEL_TC_REG(idx, reg) (ATMEL_TC_CHAN(idx) + ATMEL_TC_ ## reg)
+
+#define ATMEL_TC_CCR 0x00 /* Channel Control Register */
+#define ATMEL_TC_CLKEN (1 << 0) /* clock enable */
+#define ATMEL_TC_CLKDIS (1 << 1) /* clock disable */
+#define ATMEL_TC_SWTRG (1 << 2) /* software trigger */
+
+#define ATMEL_TC_CMR 0x04 /* Channel Mode Register */
+
+/* Both modes share some CMR bits */
+#define ATMEL_TC_TCCLKS (7 << 0) /* clock source */
+#define ATMEL_TC_TIMER_CLOCK1 (0 << 0)
+#define ATMEL_TC_TIMER_CLOCK2 (1 << 0)
+#define ATMEL_TC_TIMER_CLOCK3 (2 << 0)
+#define ATMEL_TC_TIMER_CLOCK4 (3 << 0)
+#define ATMEL_TC_TIMER_CLOCK5 (4 << 0)
+#define ATMEL_TC_XC0 (5 << 0)
+#define ATMEL_TC_XC1 (6 << 0)
+#define ATMEL_TC_XC2 (7 << 0)
+#define ATMEL_TC_CLKI (1 << 3) /* clock invert */
+#define ATMEL_TC_BURST (3 << 4) /* clock gating */
+#define ATMEL_TC_GATE_NONE (0 << 4)
+#define ATMEL_TC_GATE_XC0 (1 << 4)
+#define ATMEL_TC_GATE_XC1 (2 << 4)
+#define ATMEL_TC_GATE_XC2 (3 << 4)
+#define ATMEL_TC_WAVE (1 << 15) /* true = Waveform mode */
+
+/* CAPTURE mode CMR bits */
+#define ATMEL_TC_LDBSTOP (1 << 6) /* counter stops on RB load */
+#define ATMEL_TC_LDBDIS (1 << 7) /* counter disable on RB load */
+#define ATMEL_TC_ETRGEDG (3 << 8) /* external trigger edge */
+#define ATMEL_TC_ETRGEDG_NONE (0 << 8)
+#define ATMEL_TC_ETRGEDG_RISING (1 << 8)
+#define ATMEL_TC_ETRGEDG_FALLING (2 << 8)
+#define ATMEL_TC_ETRGEDG_BOTH (3 << 8)
+#define ATMEL_TC_ABETRG (1 << 10) /* external trigger is TIOA? */
+#define ATMEL_TC_CPCTRG (1 << 14) /* RC compare trigger enable */
+#define ATMEL_TC_LDRA (3 << 16) /* RA loading edge (of TIOA) */
+#define ATMEL_TC_LDRA_NONE (0 << 16)
+#define ATMEL_TC_LDRA_RISING (1 << 16)
+#define ATMEL_TC_LDRA_FALLING (2 << 16)
+#define ATMEL_TC_LDRA_BOTH (3 << 16)
+#define ATMEL_TC_LDRB (3 << 18) /* RB loading edge (of TIOA) */
+#define ATMEL_TC_LDRB_NONE (0 << 18)
+#define ATMEL_TC_LDRB_RISING (1 << 18)
+#define ATMEL_TC_LDRB_FALLING (2 << 18)
+#define ATMEL_TC_LDRB_BOTH (3 << 18)
+
+/* WAVEFORM mode CMR bits */
+#define ATMEL_TC_CPCSTOP (1 << 6) /* RC compare stops counter */
+#define ATMEL_TC_CPCDIS (1 << 7) /* RC compare disables counter */
+#define ATMEL_TC_EEVTEDG (3 << 8) /* external event edge */
+#define ATMEL_TC_EEVTEDG_NONE (0 << 8)
+#define ATMEL_TC_EEVTEDG_RISING (1 << 8)
+#define ATMEL_TC_EEVTEDG_FALLING (2 << 8)
+#define ATMEL_TC_EEVTEDG_BOTH (3 << 8)
+#define ATMEL_TC_EEVT (3 << 10) /* external event source */
+#define ATMEL_TC_EEVT_TIOB (0 << 10)
+#define ATMEL_TC_EEVT_XC0 (1 << 10)
+#define ATMEL_TC_EEVT_XC1 (2 << 10)
+#define ATMEL_TC_EEVT_XC2 (3 << 10)
+#define ATMEL_TC_ENETRG (1 << 12) /* external event is trigger */
+#define ATMEL_TC_WAVESEL (3 << 13) /* waveform type */
+#define ATMEL_TC_WAVESEL_UP (0 << 13)
+#define ATMEL_TC_WAVESEL_UPDOWN (1 << 13)
+#define ATMEL_TC_WAVESEL_UP_AUTO (2 << 13)
+#define ATMEL_TC_WAVESEL_UPDOWN_AUTO (3 << 13)
+#define ATMEL_TC_ACPA (3 << 16) /* RA compare changes TIOA */
+#define ATMEL_TC_ACPA_NONE (0 << 16)
+#define ATMEL_TC_ACPA_SET (1 << 16)
+#define ATMEL_TC_ACPA_CLEAR (2 << 16)
+#define ATMEL_TC_ACPA_TOGGLE (3 << 16)
+#define ATMEL_TC_ACPC (3 << 18) /* RC compare changes TIOA */
+#define ATMEL_TC_ACPC_NONE (0 << 18)
+#define ATMEL_TC_ACPC_SET (1 << 18)
+#define ATMEL_TC_ACPC_CLEAR (2 << 18)
+#define ATMEL_TC_ACPC_TOGGLE (3 << 18)
+#define ATMEL_TC_AEEVT (3 << 20) /* external event changes TIOA */
+#define ATMEL_TC_AEEVT_NONE (0 << 20)
+#define ATMEL_TC_AEEVT_SET (1 << 20)
+#define ATMEL_TC_AEEVT_CLEAR (2 << 20)
+#define ATMEL_TC_AEEVT_TOGGLE (3 << 20)
+#define ATMEL_TC_ASWTRG (3 << 22) /* software trigger changes TIOA */
+#define ATMEL_TC_ASWTRG_NONE (0 << 22)
+#define ATMEL_TC_ASWTRG_SET (1 << 22)
+#define ATMEL_TC_ASWTRG_CLEAR (2 << 22)
+#define ATMEL_TC_ASWTRG_TOGGLE (3 << 22)
+#define ATMEL_TC_BCPB (3 << 24) /* RB compare changes TIOB */
+#define ATMEL_TC_BCPB_NONE (0 << 24)
+#define ATMEL_TC_BCPB_SET (1 << 24)
+#define ATMEL_TC_BCPB_CLEAR (2 << 24)
+#define ATMEL_TC_BCPB_TOGGLE (3 << 24)
+#define ATMEL_TC_BCPC (3 << 26) /* RC compare changes TIOB */
+#define ATMEL_TC_BCPC_NONE (0 << 26)
+#define ATMEL_TC_BCPC_SET (1 << 26)
+#define ATMEL_TC_BCPC_CLEAR (2 << 26)
+#define ATMEL_TC_BCPC_TOGGLE (3 << 26)
+#define ATMEL_TC_BEEVT (3 << 28) /* external event changes TIOB */
+#define ATMEL_TC_BEEVT_NONE (0 << 28)
+#define ATMEL_TC_BEEVT_SET (1 << 28)
+#define ATMEL_TC_BEEVT_CLEAR (2 << 28)
+#define ATMEL_TC_BEEVT_TOGGLE (3 << 28)
+#define ATMEL_TC_BSWTRG (3 << 30) /* software trigger changes TIOB */
+#define ATMEL_TC_BSWTRG_NONE (0 << 30)
+#define ATMEL_TC_BSWTRG_SET (1 << 30)
+#define ATMEL_TC_BSWTRG_CLEAR (2 << 30)
+#define ATMEL_TC_BSWTRG_TOGGLE (3 << 30)
+
+#define ATMEL_TC_CV 0x10 /* counter Value */
+#define ATMEL_TC_RA 0x14 /* register A */
+#define ATMEL_TC_RB 0x18 /* register B */
+#define ATMEL_TC_RC 0x1c /* register C */
+
+#define ATMEL_TC_SR 0x20 /* status (read-only) */
+/* Status-only flags */
+#define ATMEL_TC_CLKSTA (1 << 16) /* clock enabled */
+#define ATMEL_TC_MTIOA (1 << 17) /* TIOA mirror */
+#define ATMEL_TC_MTIOB (1 << 18) /* TIOB mirror */
+
+#define ATMEL_TC_IER 0x24 /* interrupt enable (write-only) */
+#define ATMEL_TC_IDR 0x28 /* interrupt disable (write-only) */
+#define ATMEL_TC_IMR 0x2c /* interrupt mask (read-only) */
+
+/* Status and IRQ flags */
+#define ATMEL_TC_COVFS (1 << 0) /* counter overflow */
+#define ATMEL_TC_LOVRS (1 << 1) /* load overrun */
+#define ATMEL_TC_CPAS (1 << 2) /* RA compare */
+#define ATMEL_TC_CPBS (1 << 3) /* RB compare */
+#define ATMEL_TC_CPCS (1 << 4) /* RC compare */
+#define ATMEL_TC_LDRAS (1 << 5) /* RA loading */
+#define ATMEL_TC_LDRBS (1 << 6) /* RB loading */
+#define ATMEL_TC_ETRGS (1 << 7) /* external trigger */
+
+#endif
On Fri, 22 Feb 2008 17:23:23 -0800
David Brownell <[email protected]> wrote:
> Create <linux/atmel_tc.h> based on <asm-arm/arch-at91/at91-tc.h> and the
> at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
> or two of these TC blocks, which include three 16-bit timers that can be
> interconnected in various ways.
>
> These TC blocks can be used for external interfacing (such as PWM and
> measurement), or used as somewhat quirky sixteen-bit timers.
>
> Signed-off-by: David Brownell <[email protected]>
> ---
Sorry for the delay...some late comments coming up.
> Note that this won't be usable until the AT91 and AT32 platforms
> incorporate patches to configure the relevant platform devices.
> Those changes are probably 2.6.26 material.
Right...and there are a few funny dependencies we need to watch out
for. AVR32 currently uses one of the TC blocks as a system timer. That
code needs to be ripped out, but that will cause a fourfold increase in
the power consumption of the AP7000 CPU. So I want to keep the window
between ripping out the old code and using the new code as small as
possible.
I see the following possibilities:
* I switch avr32 over to use the new code after it has been merged
into mainline. This means the new code won't be tested on avr32
until near the end of the next merge window -- not good.
* I forward the patches that switch avr32 over to Andrew so that they
can be included in -mm right after the tclib support. Not a bad
option, but I'm planning to do more AVR32 PM work before 2.6.26, so
there might be conflicts.
* I take the whole thing through my avr32 git tree.
* I (or someone else) set up a new git tree for tclib + AT91 and
AVR32 platform support and ask Stephen to pull it into the -next
tree. Or, once it's stable, rmk and I can both pull it into our
respective trees. But that obviously means no rebasing and funny
games from that point on...
I think the last option is the best one. What do you think?
> +#if defined(CONFIG_AVR32)
> +/* AVR32 has these divide PBB */
> +const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
> +EXPORT_SYMBOL(atmel_tc_divisors);
> +
> +#elif defined(CONFIG_ARCH_AT91)
> +/* AT91 has these divide MCK */
> +const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
> +EXPORT_SYMBOL(atmel_tc_divisors);
> +
> +#endif
> +
> +/* we "know" that there will be either one or two TC blocks */
> +static struct platform_device *blocks[2];
You seem to know more about future Atmel chips than I do :-P
I'm not a huge fan of such static limitations. Is there any way we can
turn it into a list? That probably means defining a struct atmel_tc
around each platform_device, but that might be nice to have for other
reasons too.
> +/**
> + * atmel_tc_alloc - allocate a specified TC block
> + * @block: which block to allocate
> + * @iomem: used to return its IO memory resource
> + *
> + * Caller allocates a block. If it is available, its I/O space is requested
> + * and returned through the iomem pointer, and the device node for the block
> + * is returned. When it is not available, NULL is returned.
Any reason why it can't ioremap() the registers as well? Most drivers
probably want that.
> + * On some platforms, each TC channel has its own clocks and IRQs. Drivers
> + * should clk_get() and clk_enable() "t0_clk", "t1_clk, and "t2_clk".
> + * In the same vein, they should platform_get_irq() for irqs 0, 1, and 2.
> + * On other platforms, only irq 0 and "t0_clk" will be available; drivers
> + * should handle with both configurations.
Sounds complicated. I think it would be better if tclib did all the
clk_get() calls and stored each clock into the atmel_tc struct so that
the driver can simply clk_enable() each channel (which may point to the
same clock.)
> +struct platform_device *atmel_tc_alloc(unsigned block, struct resource **iomem)
> +{
> + struct platform_device *tc;
> + struct resource *r;
> +
> + if (block >= ARRAY_SIZE(blocks) || !iomem)
> + return NULL;
> +
> + tc = blocks[block];
> + if (tc) {
> + r = platform_get_resource(tc, IORESOURCE_MEM, 0);
> + if (r)
> + r = request_mem_region(r->start, 256, NULL);
Shouldn't you use the real resource size instead of some magic number?
> +static struct platform_driver tc_driver = {
> + .driver.name = "atmel_tcb",
Wow, I didn't know that was possible...
> +#define ATMEL_TC_BMR 0xc4 /* TC Block Mode Register */
> +#define ATMEL_TC_TC0XC0S (3 << 0) /* external clock 0 source */
> +#define ATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
Hmm. Indentation using spaces? I didn't know you were into that sort of
thing ;-)
Anyway, my main issue is that I think we should add a data structure
with information about each device, similar to struct ssc_device in the
atmel-ssc driver. How about something like this?
struct atmel_tc_block {
void __iomem *regs; /* non-NULL when busy */
struct platform_device *pdev;
struct clk *clk[3];
struct list_head node;
};
Haavard
On Sun, 24 Feb 2008 18:45:54 +0100
Haavard Skinnemoen <[email protected]> wrote:
>
> On Fri, 22 Feb 2008 17:23:23 -0800
> David Brownell <[email protected]> wrote:
>
> > Create <linux/atmel_tc.h> based on <asm-arm/arch-at91/at91-tc.h> and the
> > at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
> > or two of these TC blocks, which include three 16-bit timers that can be
> > interconnected in various ways.
> >
> > These TC blocks can be used for external interfacing (such as PWM and
> > measurement), or used as somewhat quirky sixteen-bit timers.
> >
> > Signed-off-by: David Brownell <[email protected]>
> > ---
>
> Sorry for the delay...some late comments coming up.
Happens. :)
> > Note that this won't be usable until the AT91 and AT32 platforms
> > incorporate patches to configure the relevant platform devices.
> > Those changes are probably 2.6.26 material.
More specifically (and all those patches are available now):
- AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
- AVR32 needs more extensive clocksource/clockevent updates;
- Both also need platform device setup;
Merging these two patches before those is a perfectly safe NOP.
> Right...and there are a few funny dependencies we need to watch out
> for. AVR32 currently uses one of the TC blocks as a system timer. That
> code needs to be ripped out, but that will cause a fourfold increase in
> the power consumption of the AP7000 CPU.
Because the AVR32 count/compare clocksource (architectural registers)
precludes using the "idle" state in the idle loop ... it counts CPU
cycles, which don't happen in the "idle" state.
> So I want to keep the window
> between ripping out the old code and using the new code as small as
> possible.
Right.
> I see the following possibilities:
> * I switch avr32 over to use the new code after it has been merged
> into mainline. This means the new code won't be tested on avr32
> until near the end of the next merge window -- not good.
Well, "more widely" tested. We've both observed it to work; basically
the same code has worked on AT91 for about a year now, too.
And that's why I suggest merging these parts, now that they're known to
work on both architectures. The arch-specific bits can follow at their
own pace, neither one blocking the other.
> * I forward the patches that switch avr32 over to Andrew so that they
> can be included in -mm right after the tclib support. Not a bad
> option, but I'm planning to do more AVR32 PM work before 2.6.26, so
> there might be conflicts.
Conflicts there would be the easy part to deal with. They're all
specific to AVR32, and you can merge those in any convenient order.
> * I take the whole thing through my avr32 git tree.
Doesn't work as smoothly for the AT91 side ... but Andrew Victor has
very limited time any more for such stuff (these AT91 patches will
go through him first then Russell) so I'm not sure any added delays
there could hurt much.
> * I (or someone else) set up a new git tree for tclib + AT91 and
> AVR32 platform support and ask Stephen to pull it into the -next
> tree. Or, once it's stable, rmk and I can both pull it into our
> respective trees. But that obviously means no rebasing and funny
> games from that point on...
>
> I think the last option is the best one. What do you think?
In effect, I've had that last option as a series of quilt patches,
and posting these two is the first step in that merge sequence...
heck, they could merge right now into 2.6.25-rc3 and that would let
the two arch series merge at their own paces. (AVR32 direct from
you, AT91 very indirectly through Andrew then Russell.)
My personal priority is to get these patches into the merge queue(s)
so that they're no longer sitting on my disks and so that when I
want those platforms to have HRT and/or NO_HZ, it's already there.
> > +#if defined(CONFIG_AVR32)
> > +/* AVR32 has these divide PBB */
> > +const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
> > +EXPORT_SYMBOL(atmel_tc_divisors);
> > +
> > +#elif defined(CONFIG_ARCH_AT91)
> > +/* AT91 has these divide MCK */
> > +const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
> > +EXPORT_SYMBOL(atmel_tc_divisors);
> > +
> > +#endif
> > +
> > +/* we "know" that there will be either one or two TC blocks */
> > +static struct platform_device *blocks[2];
>
> You seem to know more about future Atmel chips than I do :-P
I only "know" about currently announced ones. ;)
> I'm not a huge fan of such static limitations.
Me either, but at this point doing more than that seems to me
to land in that "overengineering" category...
> > +/**
> > + * atmel_tc_alloc - allocate a specified TC block
> > + * @block: which block to allocate
> > + * @iomem: used to return its IO memory resource
> > + *
> > + * Caller allocates a block. If it is available, its I/O space is requested
> > + * and returned through the iomem pointer, and the device node for the block
> > + * is returned. When it is not available, NULL is returned.
>
> Any reason why it can't ioremap() the registers as well? Most drivers
> probably want that.
An earlier version of the code did that. If you want to modify the
patch series to work that way, feel free. (Right now it'd be only
these two patches posted to LKML ... nobody's posted a driver for
e.g. 3-phase motor control, or servos with feedback loops, yet.)
I take it you don't have any real problem with non-support for
allocating a single TC channel. These TC blocks are quirky, and
it's a bit awkward to use just one at at time. (And that's not
only because having just sixteen bits for a timer is Not Enough!)
> > + * On some platforms, each TC channel has its own clocks and IRQs. Drivers
> > + * should clk_get() and clk_enable() "t0_clk", "t1_clk, and "t2_clk".
> > + * In the same vein, they should platform_get_irq() for irqs 0, 1, and 2.
> > + * On other platforms, only irq 0 and "t0_clk" will be available; drivers
> > + * should handle with both configurations.
>
> Sounds complicated. I think it would be better if tclib did all the
> clk_get() calls and stored each clock into the atmel_tc struct so that
> the driver can simply clk_enable() each channel (which may point to the
> same clock.)
An earlier version of the code did something like that ... ;)
In that case the platform setup code handed a block of data to
the TC library code. That was more complicated than I wanted.
You're suggesting the TC library code does that instead, and
shift that issue to a mid-layer?
In practice, this version didn't end up being hard to use. All
I really noticed was the much-simplified platform support, which
came from using platform_device resources in the "usual way".
> > +struct platform_device *atmel_tc_alloc(unsigned block, struct resource **iomem)
> > +{
> > + struct platform_device *tc;
> > + struct resource *r;
> > +
> > + if (block >= ARRAY_SIZE(blocks) || !iomem)
> > + return NULL;
> > +
> > + tc = blocks[block];
> > + if (tc) {
> > + r = platform_get_resource(tc, IORESOURCE_MEM, 0);
> > + if (r)
> > + r = request_mem_region(r->start, 256, NULL);
>
> Shouldn't you use the real resource size instead of some magic number?
The chip specs say there are only 256 bytes of registers there,
no matter how the address decoding works. It could be argued either
way, but computing the "real resource size' involves avoidable math.
> > +static struct platform_driver tc_driver = {
> > + .driver.name = "atmel_tcb",
>
> Wow, I didn't know that was possible...
Concision is my friend. ;)
> > +#define ATMEL_TC_BMR 0xc4 /* TC Block Mode Register */
> > +#define ATMEL_TC_TC0XC0S (3 << 0) /* external clock 0 source */
> > +#define ATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
>
> Hmm. Indentation using spaces? I didn't know you were into that sort of
> thing ;-)
It's way better than indenting off the right margin of the page!
> Anyway, my main issue is that I think we should add a data structure
> with information about each device, similar to struct ssc_device in the
> atmel-ssc driver. How about something like this?
>
> struct atmel_tc_block {
> void __iomem *regs; /* non-NULL when busy */
> struct platform_device *pdev;
> struct clk *clk[3];
> struct list_head node;
> };
And per-channel IRQs too...
Well, if you want to take these patches off my hands and then be
responsible for merging them upstream, I won't object. :)
- Dave
On Sun, 24 Feb 2008 14:55:27 -0800
David Brownell <[email protected]> wrote:
> On Sun, 24 Feb 2008 18:45:54 +0100
> Haavard Skinnemoen <[email protected]> wrote:
> >
> > On Fri, 22 Feb 2008 17:23:23 -0800
> > David Brownell <[email protected]> wrote:
> >
> > > Note that this won't be usable until the AT91 and AT32 platforms
> > > incorporate patches to configure the relevant platform devices.
> > > Those changes are probably 2.6.26 material.
>
> More specifically (and all those patches are available now):
>
> - AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
> - AVR32 needs more extensive clocksource/clockevent updates;
Which reminds me...you were talking about a patch that adds oneshot
support for the count/compare clocksource and more cleanups, but I
don't think I've seen it...?
I've probably messed things up enough that it won't apply cleanly
anymore, but if you just send me what you have, I'll rebase and fold it
as necessary.
> - Both also need platform device setup;
Right. I think I've got both those patches.
> Merging these two patches before those is a perfectly safe NOP.
Right. I just want to get everything properly queued up before the
merge window opens.
> > I see the following possibilities:
> > * I switch avr32 over to use the new code after it has been merged
> > into mainline. This means the new code won't be tested on avr32
> > until near the end of the next merge window -- not good.
>
> Well, "more widely" tested. We've both observed it to work; basically
> the same code has worked on AT91 for about a year now, too.
Sure, but a bit more exposure won't hurt.
> And that's why I suggest merging these parts, now that they're known to
> work on both architectures. The arch-specific bits can follow at their
> own pace, neither one blocking the other.
>
> > * I forward the patches that switch avr32 over to Andrew so that they
> > can be included in -mm right after the tclib support. Not a bad
> > option, but I'm planning to do more AVR32 PM work before 2.6.26, so
> > there might be conflicts.
>
> Conflicts there would be the easy part to deal with. They're all
> specific to AVR32, and you can merge those in any convenient order.
Assuming the tclib stuff comes first, yes. I'm not worried about me
having to deal with conflicts, I'm worried about Andrew or Stephen
having to fix up my mess.
> > * I take the whole thing through my avr32 git tree.
>
> Doesn't work as smoothly for the AT91 side ... but Andrew Victor has
> very limited time any more for such stuff (these AT91 patches will
> go through him first then Russell) so I'm not sure any added delays
> there could hurt much.
Right. I was just mentioning it as one option.
> > * I (or someone else) set up a new git tree for tclib + AT91 and
> > AVR32 platform support and ask Stephen to pull it into the -next
> > tree. Or, once it's stable, rmk and I can both pull it into our
> > respective trees. But that obviously means no rebasing and funny
> > games from that point on...
> >
> > I think the last option is the best one. What do you think?
>
> In effect, I've had that last option as a series of quilt patches,
> and posting these two is the first step in that merge sequence...
> heck, they could merge right now into 2.6.25-rc3 and that would let
> the two arch series merge at their own paces. (AVR32 direct from
> you, AT91 very indirectly through Andrew then Russell.)
Yes, getting the fundamental stuff into mainline now would help a lot.
But I was thinking about Linus' suggestions that we exploit the
distributed nature of git and do cross-tree merges to synchronize
changes to common code.
Setting up a separate git tree would allow the changes to go into the
arm tree without littering it with possibly unstable avr32 changes as
well, and it would allow me to merge them and put more stuff on top.
And if the patches are ready for mainline, there should be no need to
fix up the history of the "tclib" tree later, so it should be perfectly
safe to merge into several trees (assuming those trees don't rebase and
make them appear as different commits.)
> My personal priority is to get these patches into the merge queue(s)
> so that they're no longer sitting on my disks and so that when I
> want those platforms to have HRT and/or NO_HZ, it's already there.
Which goes nicely along with my priority of reducing the overall power
consumption on avr32 :)
> > > +/* we "know" that there will be either one or two TC blocks */
> > > +static struct platform_device *blocks[2];
> >
> > You seem to know more about future Atmel chips than I do :-P
>
> I only "know" about currently announced ones. ;)
Yeah, I didn't actually check how many TC blocks are planned on other
chips. I just want this to be a bit future-proof.
> > I'm not a huge fan of such static limitations.
>
> Me either, but at this point doing more than that seems to me
> to land in that "overengineering" category...
I don't entirely agree. Most drivers I've dealt with can handle an
arbitrary number of devices. I don't see why this one should be
different.
> > Any reason why it can't ioremap() the registers as well? Most drivers
> > probably want that.
>
> An earlier version of the code did that. If you want to modify the
> patch series to work that way, feel free. (Right now it'd be only
> these two patches posted to LKML ... nobody's posted a driver for
> e.g. 3-phase motor control, or servos with feedback loops, yet.)
Right, that's why it's probably best to change it now rather than later.
> I take it you don't have any real problem with non-support for
> allocating a single TC channel. These TC blocks are quirky, and
> it's a bit awkward to use just one at at time. (And that's not
> only because having just sixteen bits for a timer is Not Enough!)
No, I have no problems with that.
> In that case the platform setup code handed a block of data to
> the TC library code. That was more complicated than I wanted.
> You're suggesting the TC library code does that instead, and
> shift that issue to a mid-layer?
I'm suggesting that the mid-layer returns a nice struct with all the
information that the driver needs. atmel_tc_alloc() already returns two
separate values, and it might be useful to provide even more.
As an added bonus, the struct may contain a list_head so that we can
support an arbitrary number of devices as well.
> In practice, this version didn't end up being hard to use. All
> I really noticed was the much-simplified platform support, which
> came from using platform_device resources in the "usual way".
I'm not suggesting we change the platform interface. That part looks
good to me.
> > Shouldn't you use the real resource size instead of some magic number?
>
> The chip specs say there are only 256 bytes of registers there,
> no matter how the address decoding works. It could be argued either
> way, but computing the "real resource size' involves avoidable math.
Good point.
> > > +#define ATMEL_TC_BMR 0xc4 /* TC Block Mode Register */
> > > +#define ATMEL_TC_TC0XC0S (3 << 0) /* external clock 0 source */
> > > +#define ATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
> >
> > Hmm. Indentation using spaces? I didn't know you were into that sort of
> > thing ;-)
>
> It's way better than indenting off the right margin of the page!
I've never really seen the point of indenting those defines at all.
> > Anyway, my main issue is that I think we should add a data structure
> > with information about each device, similar to struct ssc_device in the
> > atmel-ssc driver. How about something like this?
> >
> > struct atmel_tc_block {
> > void __iomem *regs; /* non-NULL when busy */
> > struct platform_device *pdev;
> > struct clk *clk[3];
> > struct list_head node;
> > };
>
> And per-channel IRQs too...
I thought about that, but while the driver can safely call clk_enable()
on the same clock several times, I'm not sure if it's such a great idea
to call request_irq() on the same interrupt several times. So the
driver probably needs to know how many irqs there really are and might
as well use platform_get_irq() to find out.
> Well, if you want to take these patches off my hands and then be
> responsible for merging them upstream, I won't object. :)
I can do that.
It's getting late around here...I'll reply to your other mail tomorrow.
Haavard
> > > > Note that this won't be usable until the AT91 and AT32 platforms
> > > > incorporate patches to configure the relevant platform devices.
> > > > Those changes are probably 2.6.26 material.
> >
> > More specifically (and all those patches are available now):
> >
> > - AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
> > - AVR32 needs more extensive clocksource/clockevent updates;
>
> Which reminds me...you were talking about a patch that adds oneshot
> support for the count/compare clocksource and more cleanups, but I
> don't think I've seen it...?
I avoid sending non-working patches, and hadn't made time to
work on that issue recently.
> Yes, getting the fundamental stuff into mainline now would help a lot.
Or at least, towards the front of the merge queue, ahead of
the various platform-specific patches.
> But I was thinking about Linus' suggestions that we exploit the
> distributed nature of git and do cross-tree merges to synchronize
> changes to common code.
>
> Setting up a separate git tree would allow the changes to go into the
> arm tree without littering it with possibly unstable avr32 changes as
> well, and it would allow me to merge them and put more stuff on top.
Doing that with ARM patches is Russell's call; he hasn't been too
keen on merging from non-Linus GIT trees when that came up before.
> > > > +#define ATMEL_TC_BMR 0xc4 /* TC Block Mode Register */
> > > > +#define ATMEL_TC_TC0XC0S (3 << 0) /* external clock 0 source */
> > > > +#define ATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
> > >
> > > Hmm. Indentation using spaces? I didn't know you were into that sort of
> > > thing ;-)
> >
> > It's way better than indenting off the right margin of the page!
>
> I've never really seen the point of indenting those defines at all.
Without them, it's harder to discern the logical structure of
all the various bitfields and their contents.
> > > Anyway, my main issue is that I think we should add a data structure
> > > with information about each device, similar to struct ssc_device in the
> > > atmel-ssc driver. How about something like this?
> > >
> > > struct atmel_tc_block {
> > > void __iomem *regs; /* non-NULL when busy */
> > > struct platform_device *pdev;
> > > struct clk *clk[3];
> > > struct list_head node;
> > > };
> >
> > And per-channel IRQs too...
>
> I thought about that, but while the driver can safely call clk_enable()
> on the same clock several times, I'm not sure if it's such a great idea
> to call request_irq() on the same interrupt several times. So the
> driver probably needs to know how many irqs there really are and might
> as well use platform_get_irq() to find out.
I thought the whole point of passing the clocks was to avoid needing
to ask for them!! If trying one or three platform_get_irq() calls is
OK, then surely trying one or three clk_get() calls is also OK...
- Dave
> > Well, if you want to take these patches off my hands and then be
> > responsible for merging them upstream, I won't object. :)
>
> I can do that.
>
> It's getting late around here...I'll reply to your other mail tomorrow.
>
> Haavard
>
On Sun, 24 Feb 2008 17:03:10 -0800
David Brownell <[email protected]> wrote:
> > Which reminds me...you were talking about a patch that adds oneshot
> > support for the count/compare clocksource and more cleanups, but I
> > don't think I've seen it...?
>
> I avoid sending non-working patches, and hadn't made time to
> work on that issue recently.
I was thinking that I could perhaps help you get it working...
> > But I was thinking about Linus' suggestions that we exploit the
> > distributed nature of git and do cross-tree merges to synchronize
> > changes to common code.
> >
> > Setting up a separate git tree would allow the changes to go into the
> > arm tree without littering it with possibly unstable avr32 changes as
> > well, and it would allow me to merge them and put more stuff on top.
>
> Doing that with ARM patches is Russell's call; he hasn't been too
> keen on merging from non-Linus GIT trees when that came up before.
Fine with me either way.
> > I've never really seen the point of indenting those defines at all.
>
> Without them, it's harder to discern the logical structure of
> all the various bitfields and their contents.
I prefer to separate the registers from the bitfields and the other
stuff. That way, no indentation is necessary.
> > I thought about that, but while the driver can safely call clk_enable()
> > on the same clock several times, I'm not sure if it's such a great idea
> > to call request_irq() on the same interrupt several times. So the
> > driver probably needs to know how many irqs there really are and might
> > as well use platform_get_irq() to find out.
>
> I thought the whole point of passing the clocks was to avoid needing
> to ask for them!! If trying one or three platform_get_irq() calls is
> OK, then surely trying one or three clk_get() calls is also OK...
If you want to go down that path, surely reserving the iomem resource
is fine too? Why don't we just kill the whole tclib layer, the driver
can certainly do everything itself?
Of course the driver should be responsible for calling clk_enable() and
clk_disable(). Otherwise, power management will be tricky. And since
the driver may need to make a decision about which interrupts to
request, it might as well call platform_get_irq() directly.
On the other hand, the driver will _always_ need a reference to each
clock, and it will always need a pointer through which to access the
registers, so the mid-layer might as well do those things.
Haavard
> > > Which reminds me...you were talking about a patch that adds oneshot
> > > support for the count/compare clocksource and more cleanups, but I
> > > don't think I've seen it...?
> >
> > I avoid sending non-working patches, and hadn't made time to
> > work on that issue recently.
>
> I was thinking that I could perhaps help you get it working...
OK, if you want I'll send it.
> > > I've never really seen the point of indenting those defines at all.
> >
> > Without them, it's harder to discern the logical structure of
> > all the various bitfields and their contents.
>
> I prefer to separate the registers from the bitfields and the other
> stuff. That way, no indentation is necessary.
But that way it's no longer clear which symbols apply to which
registers. It's not "needed" because that information became
harder to find ... as in <include/asm-avr32/arch-at32ap/time.h>,
which ISTR isn't even complete in its bitfield definitions.
> > > I thought about that, but while the driver can safely call clk_enable()
> > > on the same clock several times, I'm not sure if it's such a great idea
> > > to call request_irq() on the same interrupt several times. So the
> > > driver probably needs to know how many irqs there really are and might
> > > as well use platform_get_irq() to find out.
> >
> > I thought the whole point of passing the clocks was to avoid needing
> > to ask for them!! If trying one or three platform_get_irq() calls is
> > OK, then surely trying one or three clk_get() calls is also OK...
>
> If you want to go down that path, surely reserving the iomem resource
> is fine too? Why don't we just kill the whole tclib layer, the driver
> can certainly do everything itself?
There's one clear need for tclib: to hand out timers from the (small)
pool of hardware entities. Each one can be used for different purposes
by different drivers.
The *current* tclib addresses only that minimal need.
You're the one who wants to add more to it; I was just pointing out that
you were being inconsistent.
> Of course the driver should be responsible for calling clk_enable() and
> clk_disable(). Otherwise, power management will be tricky. And since
> the driver may need to make a decision about which interrupts to
> request, it might as well call platform_get_irq() directly.
You're being inconsistent here. It has to make the same kinds of
decision about which clocks to enable/disable. So why should it
have to platform_get_irq() but not clk_get()?
> On the other hand, the driver will _always_ need a reference to each
> clock,
No; it doesn't need references to clocks for unused TC channels.
> and it will always need a pointer through which to access the
> registers, so the mid-layer might as well do those things.
True about doing the ioremap.
- Dave
On Mon, 25 Feb 2008 10:06:44 -0800
David Brownell <[email protected]> wrote:
> > > > Which reminds me...you were talking about a patch that adds oneshot
> > > > support for the count/compare clocksource and more cleanups, but I
> > > > don't think I've seen it...?
> > >
> > > I avoid sending non-working patches, and hadn't made time to
> > > work on that issue recently.
> >
> > I was thinking that I could perhaps help you get it working...
>
> OK, if you want I'll send it.
That would be great. Otherwise I fear your work might be lost...
> > > > I've never really seen the point of indenting those defines at all.
> > >
> > > Without them, it's harder to discern the logical structure of
> > > all the various bitfields and their contents.
> >
> > I prefer to separate the registers from the bitfields and the other
> > stuff. That way, no indentation is necessary.
>
> But that way it's no longer clear which symbols apply to which
> registers. It's not "needed" because that information became
> harder to find ... as in <include/asm-avr32/arch-at32ap/time.h>,
> which ISTR isn't even complete in its bitfield definitions.
Ok, I get your point.
> There's one clear need for tclib: to hand out timers from the (small)
> pool of hardware entities. Each one can be used for different purposes
> by different drivers.
>
> The *current* tclib addresses only that minimal need.
>
> You're the one who wants to add more to it; I was just pointing out that
> you were being inconsistent.
And I was trying to point out that _you_ were being inconsistent ;-)
> > Of course the driver should be responsible for calling clk_enable() and
> > clk_disable(). Otherwise, power management will be tricky. And since
> > the driver may need to make a decision about which interrupts to
> > request, it might as well call platform_get_irq() directly.
>
> You're being inconsistent here. It has to make the same kinds of
> decision about which clocks to enable/disable. So why should it
> have to platform_get_irq() but not clk_get()?
>
> > On the other hand, the driver will _always_ need a reference to each
> > clock,
>
> No; it doesn't need references to clocks for unused TC channels.
Ok. Let's drop the clock references...
> > and it will always need a pointer through which to access the
> > registers, so the mid-layer might as well do those things.
>
> True about doing the ioremap.
...and keep the regs pointer, and possibly add the iomem resource.
Ok?
Btw, I'm not saying that you're the one who has to make these changes.
Once we agree, I can send a patch to do the things we agreed on.
Haavard
On Tuesday 26 February 2008, Haavard Skinnemoen wrote:
> Ok. Let's drop the clock references...
>
> > >?????and it will always need a pointer through which to access the
> > > registers, so the mid-layer might as well do those things.
> >
> > True about doing the ioremap.
>
> ...and keep the regs pointer, and possibly add the iomem resource.
>
> Ok?
I'd be OK with clocks *and* irqs, but don't feel religious at all.
> Btw, I'm not saying that you're the one who has to make these changes.
> Once we agree, I can send a patch to do the things we agreed on.
I'll ask you to do that then, yes.
- Dave