| Message ID | dcbc38c6f1e319a31693ef0d69195fafc42a5de0.1364319776.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: > Use cdns,ttc because this driver is Cadence Rev06 > Triple Timer Counter and everybody can use it > without xilinx specific function name or probing. > > Also use standard dt description for timer > and also prepare for moving to clocksource > initialization. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > v2: Update year in copyright > Fix typo fault > Remove all zynq specific names > > Steffen: I have done this change based on our discussion. > Moving to generic location will be done in the next patch. > > Josh: We have talked about this change at ELC. > Using standard dt binding as other timers. > > I have also discussed this with Rob some time ago. > https://patchwork.kernel.org/patch/2112791/ > --- > arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ > arch/arm/boot/dts/zynq-zc702.dts | 10 -- > arch/arm/mach-zynq/common.c | 1 + > arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- > 4 files changed, 195 insertions(+), 122 deletions(-) > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > index 5914b56..51243db 100644 > --- a/arch/arm/boot/dts/zynq-7000.dtsi > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > @@ -111,56 +111,23 @@ > }; > > ttc0: ttc0@f8001000 { > - #address-cells = <1>; > - #size-cells = <0>; > - compatible = "xlnx,ttc"; > + interrupt-parent = <&intc>; > + interrupts = < 0 10 4 0 11 4 0 12 4 >; > + compatible = "cdns,ttc"; > reg = <0xF8001000 0x1000>; > clocks = <&cpu_clk 3>; > clock-names = "cpu_1x"; > clock-ranges; > - > - ttc0_0: ttc0.0 { > - status = "disabled"; > - reg = <0>; > - interrupts = <0 10 4>; > - }; > - ttc0_1: ttc0.1 { > - status = "disabled"; > - reg = <1>; > - interrupts = <0 11 4>; > - }; > - ttc0_2: ttc0.2 { > - status = "disabled"; > - reg = <2>; > - interrupts = <0 12 4>; > - }; > }; > > ttc1: ttc1@f8002000 { > - #interrupt-parent = <&intc>; > - #address-cells = <1>; > - #size-cells = <0>; > - compatible = "xlnx,ttc"; > + interrupt-parent = <&intc>; > + interrupts = < 0 37 4 0 38 4 0 39 4 >; > + compatible = "cdns,ttc"; > reg = <0xF8002000 0x1000>; > clocks = <&cpu_clk 3>; > clock-names = "cpu_1x"; > clock-ranges; > - > - ttc1_0: ttc1.0 { > - status = "disabled"; > - reg = <0>; > - interrupts = <0 37 4>; > - }; > - ttc1_1: ttc1.1 { > - status = "disabled"; > - reg = <1>; > - interrupts = <0 38 4>; > - }; > - ttc1_2: ttc1.2 { > - status = "disabled"; > - reg = <2>; > - interrupts = <0 39 4>; > - }; > }; > }; > }; Hi Michal! Thought about this a little more. I'm not seeing the improvment this gives us. The ttcs give us 6 counters that could be used however one likes. Linux wants a clocksource and clockevent provider, but I could use the Cortex timer as clocksource and would only have to sacrifice one of the 6 counters. With this patch I have to sacrifice 3 counters and would only use 2 of them. Then there is pinmuxing. That can be handled by one driver, so I think that is doable with both versions and I think I'm okay with that. So what am I missing? Why would I want it like this and not the way it is right now? Regards, Steffen > diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts > index c772942..86f44d5 100644 > --- a/arch/arm/boot/dts/zynq-zc702.dts > +++ b/arch/arm/boot/dts/zynq-zc702.dts > @@ -32,13 +32,3 @@ > &ps_clk { > clock-frequency = <33333330>; > }; > - > -&ttc0_0 { > - status = "ok"; > - compatible = "xlnx,ttc-counter-clocksource"; > -}; > - > -&ttc0_1 { > - status = "ok"; > - compatible = "xlnx,ttc-counter-clockevent"; > -}; > diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c > index 5c89832..76493b0 100644 > --- a/arch/arm/mach-zynq/common.c > +++ b/arch/arm/mach-zynq/common.c > @@ -20,6 +20,7 @@ > #include <linux/platform_device.h> > #include <linux/clk.h> > #include <linux/clk/zynq.h> > +#include <linux/clocksource.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > diff --git a/arch/arm/mach-zynq/timer.c b/arch/arm/mach-zynq/timer.c > index f9fbc9c..82357d9 100644 > --- a/arch/arm/mach-zynq/timer.c > +++ b/arch/arm/mach-zynq/timer.c > @@ -1,7 +1,7 @@ > /* > * This file contains driver for the Xilinx PS Timer Counter IP. > * > - * Copyright (C) 2011 Xilinx > + * Copyright (C) 2011-2013 Xilinx > * > * based on arch/mips/kernel/time.c timer driver > * > @@ -15,6 +15,7 @@ > * GNU General Public License for more details. > */ > > +#include <linux/clk.h> > #include <linux/interrupt.h> > #include <linux/clockchips.h> > #include <linux/of_address.h> > @@ -24,6 +25,21 @@ > #include "common.h" > > /* > + * This driver configures the 2 16-bit count-up timers as follows: > + * > + * T1: Timer 1, clocksource for generic timekeeping > + * T2: Timer 2, clockevent source for hrtimers > + * T3: Timer 3, <unused> > + * > + * The input frequency to the timer module for emulation is 2.5MHz which is > + * common to all the timer channels (T1, T2, and T3). With a pre-scaler of 32, > + * the timers are clocked at 78.125KHz (12.8 us resolution). > + > + * The input frequency to the timer module in silicon is configurable and > + * obtained from device tree. The pre-scaler of 32 is used. > + */ > + > +/* > * Timer Register Offset Definitions of Timer 1, Increment base address by 4 > * and use same offsets for Timer 2 > */ > @@ -44,17 +60,24 @@ > #define PRESCALE 2048 /* The exponent must match this */ > #define CLK_CNTRL_PRESCALE ((PRESCALE_EXPONENT - 1) << 1) > #define CLK_CNTRL_PRESCALE_EN 1 > -#define CNT_CNTRL_RESET (1<<4) > +#define CNT_CNTRL_RESET (1 << 4) > > /** > * struct xttcps_timer - This definition defines local timer structure > * > * @base_addr: Base address of timer > - **/ > + * @clk: Associated clock source > + * @clk_rate_change_nb Notifier block for clock rate changes > + */ > struct xttcps_timer { > - void __iomem *base_addr; > + void __iomem *base_addr; > + struct clk *clk; > + struct notifier_block clk_rate_change_nb; > }; > > +#define to_xttcps_timer(x) \ > + container_of(x, struct xttcps_timer, clk_rate_change_nb) > + > struct xttcps_timer_clocksource { > struct xttcps_timer xttc; > struct clocksource cs; > @@ -66,7 +89,6 @@ struct xttcps_timer_clocksource { > struct xttcps_timer_clockevent { > struct xttcps_timer xttc; > struct clock_event_device ce; > - struct clk *clk; > }; > > #define to_xttcps_timer_clkevent(x) \ > @@ -167,8 +189,8 @@ static void xttcps_set_mode(enum clock_event_mode mode, > switch (mode) { > case CLOCK_EVT_MODE_PERIODIC: > xttcps_set_interval(timer, > - DIV_ROUND_CLOSEST(clk_get_rate(xttce->clk), > - PRESCALE * HZ)); > + DIV_ROUND_CLOSEST(clk_get_rate(xttce->xttc.clk), > + PRESCALE * HZ)); > break; > case CLOCK_EVT_MODE_ONESHOT: > case CLOCK_EVT_MODE_UNUSED: > @@ -189,79 +211,148 @@ static void xttcps_set_mode(enum clock_event_mode mode, > } > } > > -static void __init zynq_ttc_setup_clocksource(struct device_node *np, > - void __iomem *base) > +static int xttcps_rate_change_clocksource_cb(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct clk_notifier_data *ndata = data; > + struct xttcps_timer *xttcps = to_xttcps_timer(nb); > + struct xttcps_timer_clocksource *xttccs = container_of(xttcps, > + struct xttcps_timer_clocksource, xttc); > + > + switch (event) { > + case POST_RATE_CHANGE: > + /* > + * Do whatever is necessary to maintain a proper time base > + * > + * I cannot find a way to adjust the currently used clocksource > + * to the new frequency. __clocksource_updatefreq_hz() sounds > + * good, but does not work. Not sure what's that missing. > + * > + * This approach works, but triggers two clocksource switches. > + * The first after unregister to clocksource jiffies. And > + * another one after the register to the newly registered timer. > + * > + * Alternatively we could 'waste' another HW timer to ping pong > + * between clock sources. That would also use one register and > + * one unregister call, but only trigger one clocksource switch > + * for the cost of another HW timer used by the OS. > + */ > + clocksource_unregister(&xttccs->cs); > + clocksource_register_hz(&xttccs->cs, > + ndata->new_rate / PRESCALE); > + /* fall through */ > + case PRE_RATE_CHANGE: > + case ABORT_RATE_CHANGE: > + default: > + return NOTIFY_DONE; > + } > +} > + > +static void __init xttc_setup_clocksource(struct clk *clk, void __iomem *base) > { > struct xttcps_timer_clocksource *ttccs; > - struct clk *clk; > int err; > - u32 reg; > > ttccs = kzalloc(sizeof(*ttccs), GFP_KERNEL); > if (WARN_ON(!ttccs)) > return; > > - err = of_property_read_u32(np, "reg", ®); > - if (WARN_ON(err)) > - return; > + ttccs->xttc.clk = clk; > > - clk = of_clk_get_by_name(np, "cpu_1x"); > - if (WARN_ON(IS_ERR(clk))) > - return; > - > - err = clk_prepare_enable(clk); > + err = clk_prepare_enable(ttccs->xttc.clk); > if (WARN_ON(err)) > return; > > - ttccs->xttc.base_addr = base + reg * 4; > + ttccs->xttc.clk_rate_change_nb.notifier_call = > + xttcps_rate_change_clocksource_cb; > + ttccs->xttc.clk_rate_change_nb.next = NULL; > + if (clk_notifier_register(ttccs->xttc.clk, > + &ttccs->xttc.clk_rate_change_nb)) > + pr_warn("Unable to register clock notifier.\n"); > > - ttccs->cs.name = np->name; > + ttccs->xttc.base_addr = base; > + ttccs->cs.name = "xttcps_clocksource"; > ttccs->cs.rating = 200; > ttccs->cs.read = __xttc_clocksource_read; > ttccs->cs.mask = CLOCKSOURCE_MASK(16); > ttccs->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS; > > + /* > + * Setup the clock source counter to be an incrementing counter > + * with no interrupt and it rolls over at 0xFFFF. Pre-scale > + * it by 32 also. Let it start running now. > + */ > __raw_writel(0x0, ttccs->xttc.base_addr + XTTCPS_IER_OFFSET); > __raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN, > ttccs->xttc.base_addr + XTTCPS_CLK_CNTRL_OFFSET); > __raw_writel(CNT_CNTRL_RESET, > ttccs->xttc.base_addr + XTTCPS_CNT_CNTRL_OFFSET); > > - err = clocksource_register_hz(&ttccs->cs, clk_get_rate(clk) / PRESCALE); > + err = clocksource_register_hz(&ttccs->cs, > + clk_get_rate(ttccs->xttc.clk) / PRESCALE); > if (WARN_ON(err)) > return; > + > } > > -static void __init zynq_ttc_setup_clockevent(struct device_node *np, > - void __iomem *base) > +static int xttcps_rate_change_clockevent_cb(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct clk_notifier_data *ndata = data; > + struct xttcps_timer *xttcps = to_xttcps_timer(nb); > + struct xttcps_timer_clockevent *xttcce = container_of(xttcps, > + struct xttcps_timer_clockevent, xttc); > + > + switch (event) { > + case POST_RATE_CHANGE: > + { > + unsigned long flags; > + > + /* > + * clockevents_update_freq should be called with IRQ disabled on > + * the CPU the timer provides events for. The timer we use is > + * common to both CPUs, not sure if we need to run on both > + * cores. > + */ > + local_irq_save(flags); > + clockevents_update_freq(&xttcce->ce, > + ndata->new_rate / PRESCALE); > + local_irq_restore(flags); > + > + /* fall through */ > + } > + case PRE_RATE_CHANGE: > + case ABORT_RATE_CHANGE: > + default: > + return NOTIFY_DONE; > + } > +} > + > +static void __init xttc_setup_clockevent(struct clk *clk, > + void __iomem *base, u32 irq) > { > struct xttcps_timer_clockevent *ttcce; > - int err, irq; > - u32 reg; > + int err; > > ttcce = kzalloc(sizeof(*ttcce), GFP_KERNEL); > if (WARN_ON(!ttcce)) > return; > > - err = of_property_read_u32(np, "reg", ®); > - if (WARN_ON(err)) > - return; > + ttcce->xttc.clk = clk; > > - ttcce->xttc.base_addr = base + reg * 4; > - > - ttcce->clk = of_clk_get_by_name(np, "cpu_1x"); > - if (WARN_ON(IS_ERR(ttcce->clk))) > - return; > - > - err = clk_prepare_enable(ttcce->clk); > + err = clk_prepare_enable(ttcce->xttc.clk); > if (WARN_ON(err)) > return; > > - irq = irq_of_parse_and_map(np, 0); > - if (WARN_ON(!irq)) > - return; > + ttcce->xttc.clk_rate_change_nb.notifier_call = > + xttcps_rate_change_clockevent_cb; > + ttcce->xttc.clk_rate_change_nb.next = NULL; > + if (clk_notifier_register(ttcce->xttc.clk, > + &ttcce->xttc.clk_rate_change_nb)) > + pr_warn("Unable to register clock notifier.\n"); > > - ttcce->ce.name = np->name; > + ttcce->xttc.base_addr = base; > + ttcce->ce.name = "xttcps_clockevent"; > ttcce->ce.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > ttcce->ce.set_next_event = xttcps_set_next_event; > ttcce->ce.set_mode = xttcps_set_mode; > @@ -269,56 +360,80 @@ static void __init zynq_ttc_setup_clockevent(struct device_node *np, > ttcce->ce.irq = irq; > ttcce->ce.cpumask = cpu_possible_mask; > > + /* > + * Setup the clock event timer to be an interval timer which > + * is prescaled by 32 using the interval interrupt. Leave it > + * disabled for now. > + */ > __raw_writel(0x23, ttcce->xttc.base_addr + XTTCPS_CNT_CNTRL_OFFSET); > __raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN, > ttcce->xttc.base_addr + XTTCPS_CLK_CNTRL_OFFSET); > __raw_writel(0x1, ttcce->xttc.base_addr + XTTCPS_IER_OFFSET); > > - err = request_irq(irq, xttcps_clock_event_interrupt, IRQF_TIMER, > - np->name, ttcce); > + err = request_irq(irq, xttcps_clock_event_interrupt, > + IRQF_DISABLED | IRQF_TIMER, > + ttcce->ce.name, ttcce); > if (WARN_ON(err)) > return; > > clockevents_config_and_register(&ttcce->ce, > - clk_get_rate(ttcce->clk) / PRESCALE, > - 1, 0xfffe); > + clk_get_rate(ttcce->xttc.clk) / PRESCALE, 1, 0xfffe); > } > > -static const __initconst struct of_device_id zynq_ttc_match[] = { > - { .compatible = "xlnx,ttc-counter-clocksource", > - .data = zynq_ttc_setup_clocksource, }, > - { .compatible = "xlnx,ttc-counter-clockevent", > - .data = zynq_ttc_setup_clockevent, }, > - {} > -}; > - > /** > * xttcps_timer_init - Initialize the timer > * > * Initializes the timer hardware and register the clock source and clock event > * timers with Linux kernal timer framework > - **/ > + */ > +static void __init xttcps_timer_init_of(struct device_node *timer) > +{ > + unsigned int irq; > + void __iomem *timer_baseaddr; > + struct clk *clk; > + > + /* > + * Get the 1st Triple Timer Counter (TTC) block from the device tree > + * and use it. Note that the event timer uses the interrupt and it's the > + * 2nd TTC hence the irq_of_parse_and_map(,1) > + */ > + timer_baseaddr = of_iomap(timer, 0); > + if (!timer_baseaddr) { > + pr_err("ERROR: invalid timer base address\n"); > + BUG(); > + } > + > + irq = irq_of_parse_and_map(timer, 1); > + if (irq <= 0) { > + pr_err("ERROR: invalid interrupt number\n"); > + BUG(); > + } > + > + clk = of_clk_get_by_name(timer, "cpu_1x"); > + if (IS_ERR(clk)) { > + pr_err("ERROR: timer input clock not found\n"); > + BUG(); > + } > + > + xttc_setup_clocksource(clk, timer_baseaddr); > + xttc_setup_clockevent(clk, timer_baseaddr + 4, irq); > + > + pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq); > +} > + > void __init xttcps_timer_init(void) > { > - struct device_node *np; > - > - for_each_compatible_node(np, NULL, "xlnx,ttc") { > - struct device_node *np_chld; > - void __iomem *base; > - > - base = of_iomap(np, 0); > - if (WARN_ON(!base)) > - return; > - > - for_each_available_child_of_node(np, np_chld) { > - int (*cb)(struct device_node *np, void __iomem *base); > - const struct of_device_id *match; > - > - match = of_match_node(zynq_ttc_match, np_chld); > - if (match) { > - cb = match->data; > - cb(np_chld, base); > - } > - } > + const char * const timer_list[] = { > + "cdns,ttc", > + NULL > + }; > + struct device_node *timer; > + > + timer = of_find_compatible_node(NULL, NULL, timer_list[0]); > + if (!timer) { > + pr_err("ERROR: no compatible timer found\n"); > + BUG(); > } > + > + xttcps_timer_init_of(timer); > } > -- > 1.7.9.7 > >
2013/3/26 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: >> Use cdns,ttc because this driver is Cadence Rev06 >> Triple Timer Counter and everybody can use it >> without xilinx specific function name or probing. >> >> Also use standard dt description for timer >> and also prepare for moving to clocksource >> initialization. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> v2: Update year in copyright >> Fix typo fault >> Remove all zynq specific names >> >> Steffen: I have done this change based on our discussion. >> Moving to generic location will be done in the next patch. >> >> Josh: We have talked about this change at ELC. >> Using standard dt binding as other timers. >> >> I have also discussed this with Rob some time ago. >> https://patchwork.kernel.org/patch/2112791/ >> --- >> arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ >> arch/arm/boot/dts/zynq-zc702.dts | 10 -- >> arch/arm/mach-zynq/common.c | 1 + >> arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- >> 4 files changed, 195 insertions(+), 122 deletions(-) >> >> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi >> index 5914b56..51243db 100644 >> --- a/arch/arm/boot/dts/zynq-7000.dtsi >> +++ b/arch/arm/boot/dts/zynq-7000.dtsi >> @@ -111,56 +111,23 @@ >> }; >> >> ttc0: ttc0@f8001000 { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "xlnx,ttc"; >> + interrupt-parent = <&intc>; >> + interrupts = < 0 10 4 0 11 4 0 12 4 >; >> + compatible = "cdns,ttc"; >> reg = <0xF8001000 0x1000>; >> clocks = <&cpu_clk 3>; >> clock-names = "cpu_1x"; >> clock-ranges; >> - >> - ttc0_0: ttc0.0 { >> - status = "disabled"; >> - reg = <0>; >> - interrupts = <0 10 4>; >> - }; >> - ttc0_1: ttc0.1 { >> - status = "disabled"; >> - reg = <1>; >> - interrupts = <0 11 4>; >> - }; >> - ttc0_2: ttc0.2 { >> - status = "disabled"; >> - reg = <2>; >> - interrupts = <0 12 4>; >> - }; >> }; >> >> ttc1: ttc1@f8002000 { >> - #interrupt-parent = <&intc>; >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "xlnx,ttc"; >> + interrupt-parent = <&intc>; >> + interrupts = < 0 37 4 0 38 4 0 39 4 >; >> + compatible = "cdns,ttc"; >> reg = <0xF8002000 0x1000>; >> clocks = <&cpu_clk 3>; >> clock-names = "cpu_1x"; >> clock-ranges; >> - >> - ttc1_0: ttc1.0 { >> - status = "disabled"; >> - reg = <0>; >> - interrupts = <0 37 4>; >> - }; >> - ttc1_1: ttc1.1 { >> - status = "disabled"; >> - reg = <1>; >> - interrupts = <0 38 4>; >> - }; >> - ttc1_2: ttc1.2 { >> - status = "disabled"; >> - reg = <2>; >> - interrupts = <0 39 4>; >> - }; >> }; >> }; >> }; > > Hi Michal! > > Thought about this a little more. I'm not seeing the improvment this gives us. > The ttcs give us 6 counters that could be used however one likes. Linux wants > a clocksource and clockevent provider, but I could use the Cortex timer as > clocksource and would only have to sacrifice one of the 6 counters. > With this patch I have to sacrifice 3 counters and would only use 2 of them. > Then there is pinmuxing. That can be handled by one driver, so I think that is > doable with both versions and I think I'm okay with that. > So what am I missing? Why would I want it like this and not the way it is right > now? There is a big move because previous DT description was incorrect. Device-tree should describe hardware and there is no something like ttc-counter-clocksource or ttc-counter-clockevent compatible driver. Every ttc counter can be used as clocksource or clockevent. Changing/Setting compatible properties for linux purpose is not correct. Just read this thread about. https://patchwork.kernel.org/patch/2112791/ Utilization of the core or using different timers in the system for clock is different discussion. We can just try to utilize timers in the arm core or add bunch of them to PL to see how kernel behaves. Thanks, Michal
On Wed, Mar 27, 2013 at 08:40:36AM +0100, Michal Simek wrote: > 2013/3/26 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > > On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: > >> Use cdns,ttc because this driver is Cadence Rev06 > >> Triple Timer Counter and everybody can use it > >> without xilinx specific function name or probing. > >> > >> Also use standard dt description for timer > >> and also prepare for moving to clocksource > >> initialization. > >> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> --- > >> v2: Update year in copyright > >> Fix typo fault > >> Remove all zynq specific names > >> > >> Steffen: I have done this change based on our discussion. > >> Moving to generic location will be done in the next patch. > >> > >> Josh: We have talked about this change at ELC. > >> Using standard dt binding as other timers. > >> > >> I have also discussed this with Rob some time ago. > >> https://patchwork.kernel.org/patch/2112791/ > >> --- > >> arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ > >> arch/arm/boot/dts/zynq-zc702.dts | 10 -- > >> arch/arm/mach-zynq/common.c | 1 + > >> arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- > >> 4 files changed, 195 insertions(+), 122 deletions(-) > >> > >> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > >> index 5914b56..51243db 100644 > >> --- a/arch/arm/boot/dts/zynq-7000.dtsi > >> +++ b/arch/arm/boot/dts/zynq-7000.dtsi > >> @@ -111,56 +111,23 @@ > >> }; > >> > >> ttc0: ttc0@f8001000 { > >> - #address-cells = <1>; > >> - #size-cells = <0>; > >> - compatible = "xlnx,ttc"; > >> + interrupt-parent = <&intc>; > >> + interrupts = < 0 10 4 0 11 4 0 12 4 >; > >> + compatible = "cdns,ttc"; > >> reg = <0xF8001000 0x1000>; > >> clocks = <&cpu_clk 3>; > >> clock-names = "cpu_1x"; > >> clock-ranges; > >> - > >> - ttc0_0: ttc0.0 { > >> - status = "disabled"; > >> - reg = <0>; > >> - interrupts = <0 10 4>; > >> - }; > >> - ttc0_1: ttc0.1 { > >> - status = "disabled"; > >> - reg = <1>; > >> - interrupts = <0 11 4>; > >> - }; > >> - ttc0_2: ttc0.2 { > >> - status = "disabled"; > >> - reg = <2>; > >> - interrupts = <0 12 4>; > >> - }; > >> }; > >> > >> ttc1: ttc1@f8002000 { > >> - #interrupt-parent = <&intc>; > >> - #address-cells = <1>; > >> - #size-cells = <0>; > >> - compatible = "xlnx,ttc"; > >> + interrupt-parent = <&intc>; > >> + interrupts = < 0 37 4 0 38 4 0 39 4 >; > >> + compatible = "cdns,ttc"; > >> reg = <0xF8002000 0x1000>; > >> clocks = <&cpu_clk 3>; > >> clock-names = "cpu_1x"; > >> clock-ranges; > >> - > >> - ttc1_0: ttc1.0 { > >> - status = "disabled"; > >> - reg = <0>; > >> - interrupts = <0 37 4>; > >> - }; > >> - ttc1_1: ttc1.1 { > >> - status = "disabled"; > >> - reg = <1>; > >> - interrupts = <0 38 4>; > >> - }; > >> - ttc1_2: ttc1.2 { > >> - status = "disabled"; > >> - reg = <2>; > >> - interrupts = <0 39 4>; > >> - }; > >> }; > >> }; > >> }; > > > > Hi Michal! > > > > Thought about this a little more. I'm not seeing the improvment this gives us. > > The ttcs give us 6 counters that could be used however one likes. Linux wants > > a clocksource and clockevent provider, but I could use the Cortex timer as > > clocksource and would only have to sacrifice one of the 6 counters. > > With this patch I have to sacrifice 3 counters and would only use 2 of them. > > Then there is pinmuxing. That can be handled by one driver, so I think that is > > doable with both versions and I think I'm okay with that. > > So what am I missing? Why would I want it like this and not the way it is right > > now? > > There is a big move because previous DT description was incorrect. > Device-tree should describe hardware and there is no something like > ttc-counter-clocksource or ttc-counter-clockevent compatible driver. > Every ttc counter can be used as clocksource or clockevent. > Changing/Setting compatible properties for linux purpose is not correct. > That is correct. DT describes the HW and the TTC includes three counters that could be used in any way a HW vendor likes. How would you decide that with just the driver? The compatible property is uncommon and might even be wrong. How about adding for example a mode-property like usb does for otg,peripheral,host? Suppose I have some obscure board with a Zynq on it, that has ttc1_0 setup to count some external signal and ttc1_1 counts some signal from the PL. I mean it is possible to configure the SoC in that way, no?! I would than have a board-specific dts where I would somehow have to describe this setup and mark the different ttcs accordingly. What I'm going for is, what is wrong in having the subnodes in ttc and then iterate over these subnodes in the driver and registering the first two ttcs that I find and that support being used as clocksource/event? > Just read this thread about. > https://patchwork.kernel.org/patch/2112791/ > Already did. > Utilization of the core or using different timers in the system for clock > is different discussion. We can just try to utilize timers in the arm core > or add bunch of them to PL to see how kernel behaves. > No problem there. I use the smp_twd on the ZedBoard and the kernel just does the right thing. Regards, Steffen
2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > On Wed, Mar 27, 2013 at 08:40:36AM +0100, Michal Simek wrote: >> 2013/3/26 Steffen Trumtrar <s.trumtrar@pengutronix.de>: >> > On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: >> >> Use cdns,ttc because this driver is Cadence Rev06 >> >> Triple Timer Counter and everybody can use it >> >> without xilinx specific function name or probing. >> >> >> >> Also use standard dt description for timer >> >> and also prepare for moving to clocksource >> >> initialization. >> >> >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> >> --- >> >> v2: Update year in copyright >> >> Fix typo fault >> >> Remove all zynq specific names >> >> >> >> Steffen: I have done this change based on our discussion. >> >> Moving to generic location will be done in the next patch. >> >> >> >> Josh: We have talked about this change at ELC. >> >> Using standard dt binding as other timers. >> >> >> >> I have also discussed this with Rob some time ago. >> >> https://patchwork.kernel.org/patch/2112791/ >> >> --- >> >> arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ >> >> arch/arm/boot/dts/zynq-zc702.dts | 10 -- >> >> arch/arm/mach-zynq/common.c | 1 + >> >> arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- >> >> 4 files changed, 195 insertions(+), 122 deletions(-) >> >> >> >> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi >> >> index 5914b56..51243db 100644 >> >> --- a/arch/arm/boot/dts/zynq-7000.dtsi >> >> +++ b/arch/arm/boot/dts/zynq-7000.dtsi >> >> @@ -111,56 +111,23 @@ >> >> }; >> >> >> >> ttc0: ttc0@f8001000 { >> >> - #address-cells = <1>; >> >> - #size-cells = <0>; >> >> - compatible = "xlnx,ttc"; >> >> + interrupt-parent = <&intc>; >> >> + interrupts = < 0 10 4 0 11 4 0 12 4 >; >> >> + compatible = "cdns,ttc"; >> >> reg = <0xF8001000 0x1000>; >> >> clocks = <&cpu_clk 3>; >> >> clock-names = "cpu_1x"; >> >> clock-ranges; >> >> - >> >> - ttc0_0: ttc0.0 { >> >> - status = "disabled"; >> >> - reg = <0>; >> >> - interrupts = <0 10 4>; >> >> - }; >> >> - ttc0_1: ttc0.1 { >> >> - status = "disabled"; >> >> - reg = <1>; >> >> - interrupts = <0 11 4>; >> >> - }; >> >> - ttc0_2: ttc0.2 { >> >> - status = "disabled"; >> >> - reg = <2>; >> >> - interrupts = <0 12 4>; >> >> - }; >> >> }; >> >> >> >> ttc1: ttc1@f8002000 { >> >> - #interrupt-parent = <&intc>; >> >> - #address-cells = <1>; >> >> - #size-cells = <0>; >> >> - compatible = "xlnx,ttc"; >> >> + interrupt-parent = <&intc>; >> >> + interrupts = < 0 37 4 0 38 4 0 39 4 >; >> >> + compatible = "cdns,ttc"; >> >> reg = <0xF8002000 0x1000>; >> >> clocks = <&cpu_clk 3>; >> >> clock-names = "cpu_1x"; >> >> clock-ranges; >> >> - >> >> - ttc1_0: ttc1.0 { >> >> - status = "disabled"; >> >> - reg = <0>; >> >> - interrupts = <0 37 4>; >> >> - }; >> >> - ttc1_1: ttc1.1 { >> >> - status = "disabled"; >> >> - reg = <1>; >> >> - interrupts = <0 38 4>; >> >> - }; >> >> - ttc1_2: ttc1.2 { >> >> - status = "disabled"; >> >> - reg = <2>; >> >> - interrupts = <0 39 4>; >> >> - }; >> >> }; >> >> }; >> >> }; >> > >> > Hi Michal! >> > >> > Thought about this a little more. I'm not seeing the improvment this gives us. >> > The ttcs give us 6 counters that could be used however one likes. Linux wants >> > a clocksource and clockevent provider, but I could use the Cortex timer as >> > clocksource and would only have to sacrifice one of the 6 counters. >> > With this patch I have to sacrifice 3 counters and would only use 2 of them. >> > Then there is pinmuxing. That can be handled by one driver, so I think that is >> > doable with both versions and I think I'm okay with that. >> > So what am I missing? Why would I want it like this and not the way it is right >> > now? >> >> There is a big move because previous DT description was incorrect. >> Device-tree should describe hardware and there is no something like >> ttc-counter-clocksource or ttc-counter-clockevent compatible driver. >> Every ttc counter can be used as clocksource or clockevent. >> Changing/Setting compatible properties for linux purpose is not correct. >> > > That is correct. DT describes the HW and the TTC includes three counters that > could be used in any way a HW vendor likes. How would you decide that with > just the driver? > The compatible property is uncommon and might even be wrong. How about adding > for example a mode-property like usb does for otg,peripheral,host? I don't think that this is right thing to do. Rob: What do you think? > Suppose I have some obscure board with a Zynq on it, that has ttc1_0 setup to > count some external signal and ttc1_1 counts some signal from the PL. I mean > it is possible to configure the SoC in that way, no?! If you are on ttc1 then it should be fine because ttc0 should be probed first and ttc1 is not allocated but look below. > I would than have a board-specific dts where I would somehow have to describe > this setup and mark the different ttcs accordingly. I understand your setup and I am not aware if there is any standard way how to handles other timers in the system to be able to work with them via standard api. What do you use? UIO for ttc setup and reading it back? > What I'm going for is, what is wrong in having the subnodes in ttc and > then iterate over these subnodes in the driver and registering the first two > ttcs that I find and that support being used as clocksource/event? I understand your concern but the question is how to describe this in dts in a proper way. Compatible property - no Based on aliases order - possible but based on my discussion with Rob probably no Additional mode options - it is also suggesting Linux specific usage. What next? > >> Just read this thread about. >> https://patchwork.kernel.org/patch/2112791/ >> > > Already did. > >> Utilization of the core or using different timers in the system for clock >> is different discussion. We can just try to utilize timers in the arm core >> or add bunch of them to PL to see how kernel behaves. >> > > No problem there. I use the smp_twd on the ZedBoard and the kernel just does the > right thing. ok. Thanks, Michal
On Wed, Mar 27, 2013 at 10:25:30AM +0100, Michal Simek wrote: > 2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > > On Wed, Mar 27, 2013 at 08:40:36AM +0100, Michal Simek wrote: > >> 2013/3/26 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > >> > On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: > >> >> Use cdns,ttc because this driver is Cadence Rev06 > >> >> Triple Timer Counter and everybody can use it > >> >> without xilinx specific function name or probing. > >> >> > >> >> Also use standard dt description for timer > >> >> and also prepare for moving to clocksource > >> >> initialization. > >> >> > >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> >> --- > >> >> v2: Update year in copyright > >> >> Fix typo fault > >> >> Remove all zynq specific names > >> >> > >> >> Steffen: I have done this change based on our discussion. > >> >> Moving to generic location will be done in the next patch. > >> >> > >> >> Josh: We have talked about this change at ELC. > >> >> Using standard dt binding as other timers. > >> >> > >> >> I have also discussed this with Rob some time ago. > >> >> https://patchwork.kernel.org/patch/2112791/ > >> >> --- > >> >> arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ > >> >> arch/arm/boot/dts/zynq-zc702.dts | 10 -- > >> >> arch/arm/mach-zynq/common.c | 1 + > >> >> arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- > >> >> 4 files changed, 195 insertions(+), 122 deletions(-) > >> >> > >> >> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > >> >> index 5914b56..51243db 100644 > >> >> --- a/arch/arm/boot/dts/zynq-7000.dtsi > >> >> +++ b/arch/arm/boot/dts/zynq-7000.dtsi > >> >> @@ -111,56 +111,23 @@ > >> >> }; > >> >> > >> >> ttc0: ttc0@f8001000 { > >> >> - #address-cells = <1>; > >> >> - #size-cells = <0>; > >> >> - compatible = "xlnx,ttc"; > >> >> + interrupt-parent = <&intc>; > >> >> + interrupts = < 0 10 4 0 11 4 0 12 4 >; > >> >> + compatible = "cdns,ttc"; > >> >> reg = <0xF8001000 0x1000>; > >> >> clocks = <&cpu_clk 3>; > >> >> clock-names = "cpu_1x"; > >> >> clock-ranges; > >> >> - > >> >> - ttc0_0: ttc0.0 { > >> >> - status = "disabled"; > >> >> - reg = <0>; > >> >> - interrupts = <0 10 4>; > >> >> - }; > >> >> - ttc0_1: ttc0.1 { > >> >> - status = "disabled"; > >> >> - reg = <1>; > >> >> - interrupts = <0 11 4>; > >> >> - }; > >> >> - ttc0_2: ttc0.2 { > >> >> - status = "disabled"; > >> >> - reg = <2>; > >> >> - interrupts = <0 12 4>; > >> >> - }; > >> >> }; > >> >> > >> >> ttc1: ttc1@f8002000 { > >> >> - #interrupt-parent = <&intc>; > >> >> - #address-cells = <1>; > >> >> - #size-cells = <0>; > >> >> - compatible = "xlnx,ttc"; > >> >> + interrupt-parent = <&intc>; > >> >> + interrupts = < 0 37 4 0 38 4 0 39 4 >; > >> >> + compatible = "cdns,ttc"; > >> >> reg = <0xF8002000 0x1000>; > >> >> clocks = <&cpu_clk 3>; > >> >> clock-names = "cpu_1x"; > >> >> clock-ranges; > >> >> - > >> >> - ttc1_0: ttc1.0 { > >> >> - status = "disabled"; > >> >> - reg = <0>; > >> >> - interrupts = <0 37 4>; > >> >> - }; > >> >> - ttc1_1: ttc1.1 { > >> >> - status = "disabled"; > >> >> - reg = <1>; > >> >> - interrupts = <0 38 4>; > >> >> - }; > >> >> - ttc1_2: ttc1.2 { > >> >> - status = "disabled"; > >> >> - reg = <2>; > >> >> - interrupts = <0 39 4>; > >> >> - }; > >> >> }; > >> >> }; > >> >> }; > >> > > >> > Hi Michal! > >> > > >> > Thought about this a little more. I'm not seeing the improvment this gives us. > >> > The ttcs give us 6 counters that could be used however one likes. Linux wants > >> > a clocksource and clockevent provider, but I could use the Cortex timer as > >> > clocksource and would only have to sacrifice one of the 6 counters. > >> > With this patch I have to sacrifice 3 counters and would only use 2 of them. > >> > Then there is pinmuxing. That can be handled by one driver, so I think that is > >> > doable with both versions and I think I'm okay with that. > >> > So what am I missing? Why would I want it like this and not the way it is right > >> > now? > >> > >> There is a big move because previous DT description was incorrect. > >> Device-tree should describe hardware and there is no something like > >> ttc-counter-clocksource or ttc-counter-clockevent compatible driver. > >> Every ttc counter can be used as clocksource or clockevent. > >> Changing/Setting compatible properties for linux purpose is not correct. > >> > > > > That is correct. DT describes the HW and the TTC includes three counters that > > could be used in any way a HW vendor likes. How would you decide that with > > just the driver? > > The compatible property is uncommon and might even be wrong. How about adding > > for example a mode-property like usb does for otg,peripheral,host? > > I don't think that this is right thing to do. > Rob: What do you think? > > > Suppose I have some obscure board with a Zynq on it, that has ttc1_0 setup to > > count some external signal and ttc1_1 counts some signal from the PL. I mean > > it is possible to configure the SoC in that way, no?! > > If you are on ttc1 then it should be fine because ttc0 should be probed first > and ttc1 is not allocated but look below. > > > I would than have a board-specific dts where I would somehow have to describe > > this setup and mark the different ttcs accordingly. > > I understand your setup and I am not aware if there is any standard > way how to handles > other timers in the system to be able to work with them via standard api. > What do you use? UIO for ttc setup and reading it back? > Oh, I don't have this setup. I'm just imagining things and how we could then handle this setup, if it ever occurs. When this binding is fixed it is fixed. At the moment it might be okay to rework this, as current mainline only works under certain circumstances and there is probably no one really using it as of yet. > > What I'm going for is, what is wrong in having the subnodes in ttc and > > then iterate over these subnodes in the driver and registering the first two > > ttcs that I find and that support being used as clocksource/event? > > I understand your concern but the question is how to describe this in > dts in a proper way. > Compatible property - no Agreed. > Based on aliases order - possible but based on my discussion with Rob > probably no Agreed. > Additional mode options - it is also suggesting Linux specific usage. > Hm, it would specify the capabilities of the ttc. Other users of the DT might be interested in this info, too. I'm not saying it is the best/only solution. > What next? > Regards, Steffen
2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > On Wed, Mar 27, 2013 at 10:25:30AM +0100, Michal Simek wrote: >> 2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: >> > On Wed, Mar 27, 2013 at 08:40:36AM +0100, Michal Simek wrote: >> >> 2013/3/26 Steffen Trumtrar <s.trumtrar@pengutronix.de>: >> >> > On Tue, Mar 26, 2013 at 06:43:33PM +0100, Michal Simek wrote: >> >> >> Use cdns,ttc because this driver is Cadence Rev06 >> >> >> Triple Timer Counter and everybody can use it >> >> >> without xilinx specific function name or probing. >> >> >> >> >> >> Also use standard dt description for timer >> >> >> and also prepare for moving to clocksource >> >> >> initialization. >> >> >> >> >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> >> >> --- >> >> >> v2: Update year in copyright >> >> >> Fix typo fault >> >> >> Remove all zynq specific names >> >> >> >> >> >> Steffen: I have done this change based on our discussion. >> >> >> Moving to generic location will be done in the next patch. >> >> >> >> >> >> Josh: We have talked about this change at ELC. >> >> >> Using standard dt binding as other timers. >> >> >> >> >> >> I have also discussed this with Rob some time ago. >> >> >> https://patchwork.kernel.org/patch/2112791/ >> >> >> --- >> >> >> arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ >> >> >> arch/arm/boot/dts/zynq-zc702.dts | 10 -- >> >> >> arch/arm/mach-zynq/common.c | 1 + >> >> >> arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- >> >> >> 4 files changed, 195 insertions(+), 122 deletions(-) >> >> >> >> >> >> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi >> >> >> index 5914b56..51243db 100644 >> >> >> --- a/arch/arm/boot/dts/zynq-7000.dtsi >> >> >> +++ b/arch/arm/boot/dts/zynq-7000.dtsi >> >> >> @@ -111,56 +111,23 @@ >> >> >> }; >> >> >> >> >> >> ttc0: ttc0@f8001000 { >> >> >> - #address-cells = <1>; >> >> >> - #size-cells = <0>; >> >> >> - compatible = "xlnx,ttc"; >> >> >> + interrupt-parent = <&intc>; >> >> >> + interrupts = < 0 10 4 0 11 4 0 12 4 >; >> >> >> + compatible = "cdns,ttc"; >> >> >> reg = <0xF8001000 0x1000>; >> >> >> clocks = <&cpu_clk 3>; >> >> >> clock-names = "cpu_1x"; >> >> >> clock-ranges; >> >> >> - >> >> >> - ttc0_0: ttc0.0 { >> >> >> - status = "disabled"; >> >> >> - reg = <0>; >> >> >> - interrupts = <0 10 4>; >> >> >> - }; >> >> >> - ttc0_1: ttc0.1 { >> >> >> - status = "disabled"; >> >> >> - reg = <1>; >> >> >> - interrupts = <0 11 4>; >> >> >> - }; >> >> >> - ttc0_2: ttc0.2 { >> >> >> - status = "disabled"; >> >> >> - reg = <2>; >> >> >> - interrupts = <0 12 4>; >> >> >> - }; >> >> >> }; >> >> >> >> >> >> ttc1: ttc1@f8002000 { >> >> >> - #interrupt-parent = <&intc>; >> >> >> - #address-cells = <1>; >> >> >> - #size-cells = <0>; >> >> >> - compatible = "xlnx,ttc"; >> >> >> + interrupt-parent = <&intc>; >> >> >> + interrupts = < 0 37 4 0 38 4 0 39 4 >; >> >> >> + compatible = "cdns,ttc"; >> >> >> reg = <0xF8002000 0x1000>; >> >> >> clocks = <&cpu_clk 3>; >> >> >> clock-names = "cpu_1x"; >> >> >> clock-ranges; >> >> >> - >> >> >> - ttc1_0: ttc1.0 { >> >> >> - status = "disabled"; >> >> >> - reg = <0>; >> >> >> - interrupts = <0 37 4>; >> >> >> - }; >> >> >> - ttc1_1: ttc1.1 { >> >> >> - status = "disabled"; >> >> >> - reg = <1>; >> >> >> - interrupts = <0 38 4>; >> >> >> - }; >> >> >> - ttc1_2: ttc1.2 { >> >> >> - status = "disabled"; >> >> >> - reg = <2>; >> >> >> - interrupts = <0 39 4>; >> >> >> - }; >> >> >> }; >> >> >> }; >> >> >> }; >> >> > >> >> > Hi Michal! >> >> > >> >> > Thought about this a little more. I'm not seeing the improvment this gives us. >> >> > The ttcs give us 6 counters that could be used however one likes. Linux wants >> >> > a clocksource and clockevent provider, but I could use the Cortex timer as >> >> > clocksource and would only have to sacrifice one of the 6 counters. >> >> > With this patch I have to sacrifice 3 counters and would only use 2 of them. >> >> > Then there is pinmuxing. That can be handled by one driver, so I think that is >> >> > doable with both versions and I think I'm okay with that. >> >> > So what am I missing? Why would I want it like this and not the way it is right >> >> > now? >> >> >> >> There is a big move because previous DT description was incorrect. >> >> Device-tree should describe hardware and there is no something like >> >> ttc-counter-clocksource or ttc-counter-clockevent compatible driver. >> >> Every ttc counter can be used as clocksource or clockevent. >> >> Changing/Setting compatible properties for linux purpose is not correct. >> >> >> > >> > That is correct. DT describes the HW and the TTC includes three counters that >> > could be used in any way a HW vendor likes. How would you decide that with >> > just the driver? >> > The compatible property is uncommon and might even be wrong. How about adding >> > for example a mode-property like usb does for otg,peripheral,host? >> >> I don't think that this is right thing to do. >> Rob: What do you think? >> >> > Suppose I have some obscure board with a Zynq on it, that has ttc1_0 setup to >> > count some external signal and ttc1_1 counts some signal from the PL. I mean >> > it is possible to configure the SoC in that way, no?! >> >> If you are on ttc1 then it should be fine because ttc0 should be probed first >> and ttc1 is not allocated but look below. >> >> > I would than have a board-specific dts where I would somehow have to describe >> > this setup and mark the different ttcs accordingly. >> >> I understand your setup and I am not aware if there is any standard >> way how to handles >> other timers in the system to be able to work with them via standard api. >> What do you use? UIO for ttc setup and reading it back? >> > > Oh, I don't have this setup. I'm just imagining things and how we could then > handle this setup, if it ever occurs. When this binding is fixed it is fixed. > At the moment it might be okay to rework this, as current mainline only works > under certain circumstances and there is probably no one really using it as of yet. > >> > What I'm going for is, what is wrong in having the subnodes in ttc and >> > then iterate over these subnodes in the driver and registering the first two >> > ttcs that I find and that support being used as clocksource/event? >> >> I understand your concern but the question is how to describe this in >> dts in a proper way. >> Compatible property - no > > Agreed. > >> Based on aliases order - possible but based on my discussion with Rob >> probably no > > Agreed. > >> Additional mode options - it is also suggesting Linux specific usage. >> > > Hm, it would specify the capabilities of the ttc. Other users of the DT > might be interested in this info, too. I'm not saying it is the best/only > solution. But the capabilities for all 2 ttc (6 timers) will be the same because they are the same and all of them can do the same things. I am definitely willing to find out any generic solution and the reason for this patch is that the current dt description is not correct and based on my discussion with Rob this was the solution which others are also using. In the valid case you have described above there are some things which can be easy to handle on zynq - if you know that the first two timers are used by linux because of the code then changing input to 3rd timer your hw design should be possible (not sure if ttc can handle input but in generic case definitely yes). But I can imagine case where the system has some limitations and for this purpose you have to use the first counter and currently you have no option how to describe this in the device-tree. It means there should be any user input to the system to say - not to use this timer for Linux purpose. And DTS is only one user input to the kernel that's why it should be written there. Describe this in chosen node? Thanks, Michal
diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi index 5914b56..51243db 100644 --- a/arch/arm/boot/dts/zynq-7000.dtsi +++ b/arch/arm/boot/dts/zynq-7000.dtsi @@ -111,56 +111,23 @@ }; ttc0: ttc0@f8001000 { - #address-cells = <1>; - #size-cells = <0>; - compatible = "xlnx,ttc"; + interrupt-parent = <&intc>; + interrupts = < 0 10 4 0 11 4 0 12 4 >; + compatible = "cdns,ttc"; reg = <0xF8001000 0x1000>; clocks = <&cpu_clk 3>; clock-names = "cpu_1x"; clock-ranges; - - ttc0_0: ttc0.0 { - status = "disabled"; - reg = <0>; - interrupts = <0 10 4>; - }; - ttc0_1: ttc0.1 { - status = "disabled"; - reg = <1>; - interrupts = <0 11 4>; - }; - ttc0_2: ttc0.2 { - status = "disabled"; - reg = <2>; - interrupts = <0 12 4>; - }; }; ttc1: ttc1@f8002000 { - #interrupt-parent = <&intc>; - #address-cells = <1>; - #size-cells = <0>; - compatible = "xlnx,ttc"; + interrupt-parent = <&intc>; + interrupts = < 0 37 4 0 38 4 0 39 4 >; + compatible = "cdns,ttc"; reg = <0xF8002000 0x1000>; clocks = <&cpu_clk 3>; clock-names = "cpu_1x"; clock-ranges; - - ttc1_0: ttc1.0 { - status = "disabled"; - reg = <0>; - interrupts = <0 37 4>; - }; - ttc1_1: ttc1.1 { - status = "disabled"; - reg = <1>; - interrupts = <0 38 4>; - }; - ttc1_2: ttc1.2 { - status = "disabled"; - reg = <2>; - interrupts = <0 39 4>; - }; }; }; }; diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts index c772942..86f44d5 100644 --- a/arch/arm/boot/dts/zynq-zc702.dts +++ b/arch/arm/boot/dts/zynq-zc702.dts @@ -32,13 +32,3 @@ &ps_clk { clock-frequency = <33333330>; }; - -&ttc0_0 { - status = "ok"; - compatible = "xlnx,ttc-counter-clocksource"; -}; - -&ttc0_1 { - status = "ok"; - compatible = "xlnx,ttc-counter-clockevent"; -}; diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c index 5c89832..76493b0 100644 --- a/arch/arm/mach-zynq/common.c +++ b/arch/arm/mach-zynq/common.c @@ -20,6 +20,7 @@ #include <linux/platform_device.h> #include <linux/clk.h> #include <linux/clk/zynq.h> +#include <linux/clocksource.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> diff --git a/arch/arm/mach-zynq/timer.c b/arch/arm/mach-zynq/timer.c index f9fbc9c..82357d9 100644 --- a/arch/arm/mach-zynq/timer.c +++ b/arch/arm/mach-zynq/timer.c @@ -1,7 +1,7 @@ /* * This file contains driver for the Xilinx PS Timer Counter IP. * - * Copyright (C) 2011 Xilinx + * Copyright (C) 2011-2013 Xilinx * * based on arch/mips/kernel/time.c timer driver * @@ -15,6 +15,7 @@ * GNU General Public License for more details. */ +#include <linux/clk.h> #include <linux/interrupt.h> #include <linux/clockchips.h> #include <linux/of_address.h> @@ -24,6 +25,21 @@ #include "common.h" /* + * This driver configures the 2 16-bit count-up timers as follows: + * + * T1: Timer 1, clocksource for generic timekeeping + * T2: Timer 2, clockevent source for hrtimers + * T3: Timer 3, <unused> + * + * The input frequency to the timer module for emulation is 2.5MHz which is + * common to all the timer channels (T1, T2, and T3). With a pre-scaler of 32, + * the timers are clocked at 78.125KHz (12.8 us resolution). + + * The input frequency to the timer module in silicon is configurable and + * obtained from device tree. The pre-scaler of 32 is used. + */ + +/* * Timer Register Offset Definitions of Timer 1, Increment base address by 4 * and use same offsets for Timer 2 */ @@ -44,17 +60,24 @@ #define PRESCALE 2048 /* The exponent must match this */ #define CLK_CNTRL_PRESCALE ((PRESCALE_EXPONENT - 1) << 1) #define CLK_CNTRL_PRESCALE_EN 1 -#define CNT_CNTRL_RESET (1<<4) +#define CNT_CNTRL_RESET (1 << 4) /** * struct xttcps_timer - This definition defines local timer structure * * @base_addr: Base address of timer - **/ + * @clk: Associated clock source + * @clk_rate_change_nb Notifier block for clock rate changes + */ struct xttcps_timer { - void __iomem *base_addr; + void __iomem *base_addr; + struct clk *clk; + struct notifier_block clk_rate_change_nb; }; +#define to_xttcps_timer(x) \ + container_of(x, struct xttcps_timer, clk_rate_change_nb) + struct xttcps_timer_clocksource { struct xttcps_timer xttc; struct clocksource cs; @@ -66,7 +89,6 @@ struct xttcps_timer_clocksource { struct xttcps_timer_clockevent { struct xttcps_timer xttc; struct clock_event_device ce; - struct clk *clk; }; #define to_xttcps_timer_clkevent(x) \ @@ -167,8 +189,8 @@ static void xttcps_set_mode(enum clock_event_mode mode, switch (mode) { case CLOCK_EVT_MODE_PERIODIC: xttcps_set_interval(timer, - DIV_ROUND_CLOSEST(clk_get_rate(xttce->clk), - PRESCALE * HZ)); + DIV_ROUND_CLOSEST(clk_get_rate(xttce->xttc.clk), + PRESCALE * HZ)); break; case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: @@ -189,79 +211,148 @@ static void xttcps_set_mode(enum clock_event_mode mode, } } -static void __init zynq_ttc_setup_clocksource(struct device_node *np, - void __iomem *base) +static int xttcps_rate_change_clocksource_cb(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct xttcps_timer *xttcps = to_xttcps_timer(nb); + struct xttcps_timer_clocksource *xttccs = container_of(xttcps, + struct xttcps_timer_clocksource, xttc); + + switch (event) { + case POST_RATE_CHANGE: + /* + * Do whatever is necessary to maintain a proper time base + * + * I cannot find a way to adjust the currently used clocksource + * to the new frequency. __clocksource_updatefreq_hz() sounds + * good, but does not work. Not sure what's that missing. + * + * This approach works, but triggers two clocksource switches. + * The first after unregister to clocksource jiffies. And + * another one after the register to the newly registered timer. + * + * Alternatively we could 'waste' another HW timer to ping pong + * between clock sources. That would also use one register and + * one unregister call, but only trigger one clocksource switch + * for the cost of another HW timer used by the OS. + */ + clocksource_unregister(&xttccs->cs); + clocksource_register_hz(&xttccs->cs, + ndata->new_rate / PRESCALE); + /* fall through */ + case PRE_RATE_CHANGE: + case ABORT_RATE_CHANGE: + default: + return NOTIFY_DONE; + } +} + +static void __init xttc_setup_clocksource(struct clk *clk, void __iomem *base) { struct xttcps_timer_clocksource *ttccs; - struct clk *clk; int err; - u32 reg; ttccs = kzalloc(sizeof(*ttccs), GFP_KERNEL); if (WARN_ON(!ttccs)) return; - err = of_property_read_u32(np, "reg", ®); - if (WARN_ON(err)) - return; + ttccs->xttc.clk = clk; - clk = of_clk_get_by_name(np, "cpu_1x"); - if (WARN_ON(IS_ERR(clk))) - return; - - err = clk_prepare_enable(clk); + err = clk_prepare_enable(ttccs->xttc.clk); if (WARN_ON(err)) return; - ttccs->xttc.base_addr = base + reg * 4; + ttccs->xttc.clk_rate_change_nb.notifier_call = + xttcps_rate_change_clocksource_cb; + ttccs->xttc.clk_rate_change_nb.next = NULL; + if (clk_notifier_register(ttccs->xttc.clk, + &ttccs->xttc.clk_rate_change_nb)) + pr_warn("Unable to register clock notifier.\n"); - ttccs->cs.name = np->name; + ttccs->xttc.base_addr = base; + ttccs->cs.name = "xttcps_clocksource"; ttccs->cs.rating = 200; ttccs->cs.read = __xttc_clocksource_read; ttccs->cs.mask = CLOCKSOURCE_MASK(16); ttccs->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS; + /* + * Setup the clock source counter to be an incrementing counter + * with no interrupt and it rolls over at 0xFFFF. Pre-scale + * it by 32 also. Let it start running now. + */ __raw_writel(0x0, ttccs->xttc.base_addr + XTTCPS_IER_OFFSET); __raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN, ttccs->xttc.base_addr + XTTCPS_CLK_CNTRL_OFFSET); __raw_writel(CNT_CNTRL_RESET, ttccs->xttc.base_addr + XTTCPS_CNT_CNTRL_OFFSET); - err = clocksource_register_hz(&ttccs->cs, clk_get_rate(clk) / PRESCALE); + err = clocksource_register_hz(&ttccs->cs, + clk_get_rate(ttccs->xttc.clk) / PRESCALE); if (WARN_ON(err)) return; + } -static void __init zynq_ttc_setup_clockevent(struct device_node *np, - void __iomem *base) +static int xttcps_rate_change_clockevent_cb(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct xttcps_timer *xttcps = to_xttcps_timer(nb); + struct xttcps_timer_clockevent *xttcce = container_of(xttcps, + struct xttcps_timer_clockevent, xttc); + + switch (event) { + case POST_RATE_CHANGE: + { + unsigned long flags; + + /* + * clockevents_update_freq should be called with IRQ disabled on + * the CPU the timer provides events for. The timer we use is + * common to both CPUs, not sure if we need to run on both + * cores. + */ + local_irq_save(flags); + clockevents_update_freq(&xttcce->ce, + ndata->new_rate / PRESCALE); + local_irq_restore(flags); + + /* fall through */ + } + case PRE_RATE_CHANGE: + case ABORT_RATE_CHANGE: + default: + return NOTIFY_DONE; + } +} + +static void __init xttc_setup_clockevent(struct clk *clk, + void __iomem *base, u32 irq) { struct xttcps_timer_clockevent *ttcce; - int err, irq; - u32 reg; + int err; ttcce = kzalloc(sizeof(*ttcce), GFP_KERNEL); if (WARN_ON(!ttcce)) return; - err = of_property_read_u32(np, "reg", ®); - if (WARN_ON(err)) - return; + ttcce->xttc.clk = clk; - ttcce->xttc.base_addr = base + reg * 4; - - ttcce->clk = of_clk_get_by_name(np, "cpu_1x"); - if (WARN_ON(IS_ERR(ttcce->clk))) - return; - - err = clk_prepare_enable(ttcce->clk); + err = clk_prepare_enable(ttcce->xttc.clk); if (WARN_ON(err)) return; - irq = irq_of_parse_and_map(np, 0); - if (WARN_ON(!irq)) - return; + ttcce->xttc.clk_rate_change_nb.notifier_call = + xttcps_rate_change_clockevent_cb; + ttcce->xttc.clk_rate_change_nb.next = NULL; + if (clk_notifier_register(ttcce->xttc.clk, + &ttcce->xttc.clk_rate_change_nb)) + pr_warn("Unable to register clock notifier.\n"); - ttcce->ce.name = np->name; + ttcce->xttc.base_addr = base; + ttcce->ce.name = "xttcps_clockevent"; ttcce->ce.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; ttcce->ce.set_next_event = xttcps_set_next_event; ttcce->ce.set_mode = xttcps_set_mode; @@ -269,56 +360,80 @@ static void __init zynq_ttc_setup_clockevent(struct device_node *np, ttcce->ce.irq = irq; ttcce->ce.cpumask = cpu_possible_mask; + /* + * Setup the clock event timer to be an interval timer which + * is prescaled by 32 using the interval interrupt. Leave it + * disabled for now. + */ __raw_writel(0x23, ttcce->xttc.base_addr + XTTCPS_CNT_CNTRL_OFFSET); __raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN, ttcce->xttc.base_addr + XTTCPS_CLK_CNTRL_OFFSET); __raw_writel(0x1, ttcce->xttc.base_addr + XTTCPS_IER_OFFSET); - err = request_irq(irq, xttcps_clock_event_interrupt, IRQF_TIMER, - np->name, ttcce); + err = request_irq(irq, xttcps_clock_event_interrupt, + IRQF_DISABLED | IRQF_TIMER, + ttcce->ce.name, ttcce); if (WARN_ON(err)) return; clockevents_config_and_register(&ttcce->ce, - clk_get_rate(ttcce->clk) / PRESCALE, - 1, 0xfffe); + clk_get_rate(ttcce->xttc.clk) / PRESCALE, 1, 0xfffe); } -static const __initconst struct of_device_id zynq_ttc_match[] = { - { .compatible = "xlnx,ttc-counter-clocksource", - .data = zynq_ttc_setup_clocksource, }, - { .compatible = "xlnx,ttc-counter-clockevent", - .data = zynq_ttc_setup_clockevent, }, - {} -}; - /** * xttcps_timer_init - Initialize the timer * * Initializes the timer hardware and register the clock source and clock event * timers with Linux kernal timer framework - **/ + */ +static void __init xttcps_timer_init_of(struct device_node *timer) +{ + unsigned int irq; + void __iomem *timer_baseaddr; + struct clk *clk; + + /* + * Get the 1st Triple Timer Counter (TTC) block from the device tree + * and use it. Note that the event timer uses the interrupt and it's the + * 2nd TTC hence the irq_of_parse_and_map(,1) + */ + timer_baseaddr = of_iomap(timer, 0); + if (!timer_baseaddr) { + pr_err("ERROR: invalid timer base address\n"); + BUG(); + } + + irq = irq_of_parse_and_map(timer, 1); + if (irq <= 0) { + pr_err("ERROR: invalid interrupt number\n"); + BUG(); + } + + clk = of_clk_get_by_name(timer, "cpu_1x"); + if (IS_ERR(clk)) { + pr_err("ERROR: timer input clock not found\n"); + BUG(); + } + + xttc_setup_clocksource(clk, timer_baseaddr); + xttc_setup_clockevent(clk, timer_baseaddr + 4, irq); + + pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq); +} + void __init xttcps_timer_init(void) { - struct device_node *np; - - for_each_compatible_node(np, NULL, "xlnx,ttc") { - struct device_node *np_chld; - void __iomem *base; - - base = of_iomap(np, 0); - if (WARN_ON(!base)) - return; - - for_each_available_child_of_node(np, np_chld) { - int (*cb)(struct device_node *np, void __iomem *base); - const struct of_device_id *match; - - match = of_match_node(zynq_ttc_match, np_chld); - if (match) { - cb = match->data; - cb(np_chld, base); - } - } + const char * const timer_list[] = { + "cdns,ttc", + NULL + }; + struct device_node *timer; + + timer = of_find_compatible_node(NULL, NULL, timer_list[0]); + if (!timer) { + pr_err("ERROR: no compatible timer found\n"); + BUG(); } + + xttcps_timer_init_of(timer); }
Use cdns,ttc because this driver is Cadence Rev06 Triple Timer Counter and everybody can use it without xilinx specific function name or probing. Also use standard dt description for timer and also prepare for moving to clocksource initialization. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- v2: Update year in copyright Fix typo fault Remove all zynq specific names Steffen: I have done this change based on our discussion. Moving to generic location will be done in the next patch. Josh: We have talked about this change at ELC. Using standard dt binding as other timers. I have also discussed this with Rob some time ago. https://patchwork.kernel.org/patch/2112791/ --- arch/arm/boot/dts/zynq-7000.dtsi | 45 +------ arch/arm/boot/dts/zynq-zc702.dts | 10 -- arch/arm/mach-zynq/common.c | 1 + arch/arm/mach-zynq/timer.c | 261 +++++++++++++++++++++++++++----------- 4 files changed, 195 insertions(+), 122 deletions(-)